HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage by hemanthumashankar0511 · Pull Request #6587 · apache/hive

hemanthumashankar0511 · 2026-07-08T10:53:28Z

What changes were proposed in this pull request?

Adds JDBC DatabaseAccessor unit and integration tests in TestGenericJdbcDatabaseAccessor and TestDatabaseAccessorByDatabaseType, plus test_script.sql fixture data, covering dialect SQL generation, factory routing, boundary queries, Hive column stripping, and H2 smoke tests across database types.

Why are the changes needed?

To improve DatabaseAccessor test coverage for GenericJdbcDatabaseAccessor as part of HIVE-26441.

Does this PR introduce any user-facing change?

No

How was this patch tested?

mvn test -pl jdbc-handler -Dtest=TestGenericJdbcDatabaseAccessor,TestDatabaseAccessorByDatabaseType

…essorByDatabaseType

Copilot

Pull request overview

This PR expands test coverage for GenericJdbcDatabaseAccessor and DatabaseAccessorFactory in the jdbc-handler module, validating SQL generation behaviors across dialect-specific accessors and adding H2-backed smoke/integration coverage.

Changes:

Added additional unit tests in TestGenericJdbcDatabaseAccessor for boundary queries, bounds retrieval, accessor quoting behavior, and record-writer persistence.
Introduced a new parameterized test suite (TestDatabaseAccessorByDatabaseType) to validate limit/offset SQL generation, factory routing, Hive column-name stripping, and H2-compatible integration checks across DatabaseTypes.
Extended the SQL fixture (test_script.sql) with a new test_writer table used by record-writer tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
jdbc-handler/src/test/resources/test_script.sql	Adds `test_writer` table fixture for record-writer tests.
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestGenericJdbcDatabaseAccessor.java	Expands unit/integration coverage for bounds, boundary SQL rewriting, quoting behavior, close behavior, and record writing.
jdbc-handler/src/test/java/org/apache/hive/storage/jdbc/dao/TestDatabaseAccessorByDatabaseType.java	New parameterized suite validating per-dialect SQL generation, factory routing, and H2-backed smoke tests across database types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    config.set(JdbcStorageConfig.DATABASE_TYPE.getPropertyName(), databaseType.name());
+    config.set(JdbcStorageConfig.JDBC_DRIVER_CLASS.getPropertyName(), H2_DRIVER);
+    config.set(JdbcStorageConfig.JDBC_URL.getPropertyName(),
+        "jdbc:h2:mem:test_dbtype_integration_" + databaseType.name().toLowerCase()


ayushtkn

Thanx @hemanthumashankar0511 dropped some comments

ayushtkn · 2026-07-22T05:36:06Z

+/*
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");


Is this licence right? I think line-2 shouldn't be there, I think the content is also diffrent from othe rfiles

/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * <p/> * http://www.apache.org/licenses/LICENSE-2.0 * <p/> * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */

Hey I checked the jdbc-handler module and the licence differs from file to file..some files have 2 lines of space and some have one line space in the beginning

hmm, that doesn't look very cheesy to me, we should ideally use one single pattern for ASF Header in the entire project, can you create a ticket to make it uniform across the project with what is mentioned over here:
https://www.apache.org/legal/src-headers.html

lets use the one mentioned in the doc for this new class

yeah sure!!

ayushtkn · 2026-07-22T05:37:58Z

+        assertThat(rs.getString("name"), is(equalTo("written")));
+        assertThat(rs.next(), is(false));
+      }
+      statement.execute("SHUTDOWN");


this should have been in the finally block, if any assertion fails, this would leak, shutdown won't be called

ayushtkn · 2026-07-22T05:38:26Z

+
+        assertThat(rs.next(), is(false));
+      }
+      statement.execute("SHUTDOWN");


same as above, it should be in finally or maybe the cleanup should be in After block

ayushtkn · 2026-07-22T05:39:16Z

+  private static Object[] row(Object... values) {
+    return values;


what is this for?

It was a tiny helper to build parameterized test rows for JUnit’s @parameters data provider, but it didn’t add any real logic. I'll remove it

hemanthumashankar0511 · 2026-07-23T17:38:58Z

Thanks @ayushtkn for the review. I have addressed the review comments. PTAL whenever possible

sonarqubecloud · 2026-07-24T05:50:23Z

Quality Gate passed

Issues
33 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ayushtkn

LGTM

HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage

0af2eaa

asf-ci-hive added tests pending tests passed and removed tests pending labels Jul 8, 2026

hemanthumashankar0511 marked this pull request as draft July 10, 2026 10:39

asf-ci-hive added tests pending and removed tests passed labels Jul 10, 2026

hemanthumashankar0511 changed the title ~~HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage~~ [WIP]HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage Jul 10, 2026

asf-ci-hive added tests passed and removed tests pending labels Jul 10, 2026

hemanthumashankar0511 added 5 commits July 13, 2026 12:00

Consolidate JDBC database accessor dialect tests into TestDatabaseAcc…

6548c29

…essorByDatabaseType

Add JDBC accessor factory mapping tests

197b04e

Add JDBC boundary query unit tests

79c56c0

Add Hive JDBC column name stripping test

70b41ca

Add JDBC accessor integration smoke tests by database type

7e698ab

hemanthumashankar0511 force-pushed the database_accessor_unit_tests branch from c813e9c to 7e698ab Compare July 13, 2026 06:35

asf-ci-hive added tests pending and removed tests passed labels Jul 13, 2026

hemanthumashankar0511 marked this pull request as ready for review July 13, 2026 07:51

hemanthumashankar0511 changed the title ~~[WIP]HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage~~ HIVE-26441: Expand GenericJdbcDatabaseAccessor unit test coverage Jul 13, 2026

asf-ci-hive added tests passed and removed tests pending labels Jul 13, 2026

ayushtkn requested review from Copilot and zabetak July 15, 2026 06:23

Copilot started reviewing on behalf of ayushtkn July 22, 2026 05:02 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

ayushtkn reviewed Jul 22, 2026

View reviewed changes

Addressing reviewer comments

c7bba65

asf-ci-hive added tests pending and removed tests passed labels Jul 22, 2026

hemanthumashankar0511 force-pushed the database_accessor_unit_tests branch from 480abe7 to c7bba65 Compare July 22, 2026 07:16

asf-ci-hive added tests failed tests pending tests unstable tests passed and removed tests pending tests failed tests unstable labels Jul 22, 2026

hemanthumashankar0511 requested a review from ayushtkn July 23, 2026 17:39

Licence header updated

acc9a8c

asf-ci-hive added tests pending and removed tests passed labels Jul 24, 2026

asf-ci-hive added tests passed and removed tests pending labels Jul 24, 2026

ayushtkn approved these changes Jul 24, 2026

View reviewed changes

		private static Object[] row(Object... values) {
		return values;

Uh oh!

Conversation

hemanthumashankar0511 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

ayushtkn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

hemanthumashankar0511 Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

hemanthumashankar0511 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

hemanthumashankar0511 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

hemanthumashankar0511 commented Jul 23, 2026

Uh oh!

sonarqubecloud Bot commented Jul 24, 2026

Quality Gate passed

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hemanthumashankar0511 commented Jul 8, 2026 •

edited

Loading

ayushtkn left a comment •

edited

Loading

hemanthumashankar0511 Jul 22, 2026 •

edited

Loading