HIVE-15540: Impl `DatabaseMetaData#getURL()` and `DatabaseMetaData#getUserName()` for HiveServer2 JDBC Driver #5554

linghengqian · 2024-11-22T13:37:13Z

What changes were proposed in this pull request?

Impl DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() for HiveServer2 JDBC Driver.

Why are the changes needed?

I totally don't understand why DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() are not implemented for HiveServer2 JDBC Driver in Support connecting to HiveServer2 through database connection pools other than HikariCP shardingsphere#33762 and Improve GraalVM Reachability Metadata and corresponding nativeTest related unit tests shardingsphere#29052 . This leads to funny logic like the following.

        try (Connection connection = dataSource.getConnection()) {
            return connection.getMetaData().getURL();
        } catch (final SQLFeatureNotSupportedException sqlFeatureNotSupportedException) {
            try (Connection connection = dataSource.getConnection()) {
                Class<?> hiveConnectionClass = Class.forName("org.apache.hive.jdbc.HiveConnection");
                if (connection.isWrapperFor(hiveConnectionClass)) {
                    Object hiveConnection = connection.unwrap(hiveConnectionClass);
                    String connectedUrl = (String) hiveConnectionClass.getMethod("getConnectedUrl").invoke(hiveConnection);
                    assertThat(connectedUrl, is("jdbc:hive2://127.0.0.1:2181/demo_ds_0;initFile=/tmp/init.sql;serviceDiscoveryMode=zooKeeper;zooKeeperNamespace=hiveserver2"));
                    return connectedUrl;
                }
                throw new SQLWrapperException(sqlFeatureNotSupportedException);
            } catch (final SQLException | ClassNotFoundException | NoSuchMethodException | InvocationTargetException | IllegalAccessException exception) {
                throw new RuntimeException(exception);
            }
        } catch (final SQLException ex) {
            throw new SQLWrapperException(ex);
        }

Does this PR introduce any user-facing change?

Maybe only third-party frameworks or tools like DBeaver will find the impact of the current PR. If one uses a web framework like Spring Boot framework, he has no way of knowing what the current PR does.

Is the change a dependency upgrade?

No

How was this patch tested?

sdk install java 8.0.422-tem
sdk use java 8.0.422-tem
sdk install maven
mvn clean install -DskipTests -Pitests
mvn test -Dtest=TestHiveDatabaseMetaData -Dtest.output.overwrite=true -pl itests/hive-unit -Pitests

InvisibleProgrammer · 2024-11-22T14:39:38Z

I just looked into the jdbc driver recently and got surprised similarly as you.

Can I ask you to add some tests around the changes? I think TestJdbcDriver can be a great place to do that.

InvisibleProgrammer · 2024-11-22T14:36:49Z

jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java

  }

  public String getUserName() throws SQLException {
-    throw new SQLFeatureNotSupportedException("Method not supported");
+    return connection.getConnParams().getSessionVars().get("user");


I think it would be more bullet-proof to get the user name similar way as HiveConnection does: It gets the username, or it is not provided, gives back the anonymous:

/** * @return username from sessConfMap */ private String getUserName() { return getSessionValue(JdbcConnectionParams.AUTH_USER, JdbcConnectionParams.ANONYMOUS_USER); }

HiveConnection#getSessionValue(String, String) and HiveConnection#getUserName() are both private methods. I guess what you mean is?

public String getUserName() throws SQLException { String username = connection.getConnParams().getSessionVars().get(JdbcConnectionParams.AUTH_USER); if ((username == null) || username.isEmpty()) { username = JdbcConnectionParams.ANONYMOUS_USER; } return username; }

Changing the org.apache.hive.jdbc.TestJdbcDriver doesn't seem intuitive enough, should I create a org.apache.hive.jdbc.TestHiveDatabaseMetaData class?

By the way, Jenkins used by Hive limits a single PR to 5 hours between two CIs, and the current PR's github tag is a bit inconsistent with the actual situation.

HiveConnection#getSessionValue(String, String) and HiveConnection#getUserName() are both private methods. I guess what you mean is?

Yes, something like that.

Changing the org.apache.hive.jdbc.TestJdbcDriver doesn't seem intuitive enough, should I create a org.apache.hive.jdbc.TestHiveDatabaseMetaData class?

Excuse me, wrong copy-paste. So, there is already a test class here: https://github.com/apache/hive/blob/master/itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestHiveDatabaseMetaData.java

I added new unit tests. I guess Hive Doc does not prohibit the use of Guava's utility classes in unit tests.

I must admit that I found the Hive Contributor Guide on ASF Confluence to be out of date, and I had to spend some time looking up how to start a single unit test locally.

sdk install java 8.0.422-tem sdk use java 8.0.422-tem sdk install maven mvn clean install -DskipTests -Pitests mvn test -Dtest=TestHiveDatabaseMetaData -Dtest.output.overwrite=true -pl itests/hive-unit -Pitests

InvisibleProgrammer

LGTM. Thank you for the change.

linghengqian

I just did a quick check of https://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/activity and found that unit tests for TestMiniLlapLocalCliDriver are failing in all recent PRs. Is there a quicker way to complete the tests mentioned in https://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/detail/PR-5554/2/tests than the following command?

sdk install java 8.0.422-tem
sdk use java 8.0.422-tem
sdk install maven
mvn clean install -DskipTests -Pitests
mvn test -Dtest=TestMiniLlapLocalCliDriver -Dtest.output.overwrite=true -pl itests/qtest -Pitests

Update in 2024.11.24: See HIVE-28636: partition_explain_ddl.q is flaky #5555 .
Update in 2024.11.25: See 2bf6614 .
Update in 2024.11.26: I just did a quick check of https://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/activity and found that unit tests for TestIcebergLlapLocalCompactorCliDriver are failing in all recent PRs.....The master branch still seems to have problems.
Update in 2024.11.27: See a79922e .

ayushtkn

Minor comments, rest looks good

ayushtkn · 2024-12-10T17:23:12Z

itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestHiveDatabaseMetaData.java

-import org.junit.Before;
-import org.junit.Test;


Can you just avoid changing the import order, just makes things complicated while backporting

ayushtkn · 2024-12-10T17:36:08Z

jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java

  public String getUserName() throws SQLException {
-    throw new SQLFeatureNotSupportedException("Method not supported");
+    String userName = connection.getConnParams().getSessionVars().get(JdbcConnectionParams.AUTH_USER);
+    if ((userName == null) || userName.isEmpty()) {
+      userName = JdbcConnectionParams.ANONYMOUS_USER;
+    }
+    return userName;
  }


can we just do

public String getUserName() throws SQLException { return connection.getUserName(); }

and make getUserName() in HiveConnection package-private (default)?

…tUserName()` for HiveServer2 JDBC Driver

sonarqubecloud · 2024-12-12T09:46:48Z

Quality Gate passed

Issues
0 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

asf-ci-hive added the tests pending label Nov 22, 2024

linghengqian mentioned this pull request Nov 22, 2024

Support connecting to HiveServer2 through database connection pools other than HikariCP apache/shardingsphere#33762

Merged

7 tasks

linghengqian force-pushed the presto-bump branch from 598a367 to 13fca4f Compare November 22, 2024 13:54

linghengqian marked this pull request as ready for review November 22, 2024 13:59

linghengqian changed the title ~~[WIP]HIVE-15540: Impl DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() for HiveServer2 JDBC Driver~~ HIVE-15540: Impl DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() for HiveServer2 JDBC Driver Nov 22, 2024

asf-ci-hive added tests failed and removed tests pending labels Nov 22, 2024

InvisibleProgrammer suggested changes Nov 22, 2024

View reviewed changes

linghengqian force-pushed the presto-bump branch from 13fca4f to 5cdac26 Compare November 22, 2024 15:45

InvisibleProgrammer approved these changes Nov 22, 2024

View reviewed changes

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending labels Nov 22, 2024

linghengqian commented Nov 23, 2024

View reviewed changes

linghengqian force-pushed the presto-bump branch from 5cdac26 to 297f166 Compare November 25, 2024 08:37

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Nov 25, 2024

linghengqian force-pushed the presto-bump branch from 297f166 to 9f2a3e2 Compare November 27, 2024 03:07

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Nov 27, 2024

linghengqian mentioned this pull request Dec 8, 2024

SQLFeatureNotSupportedException ( Hive, MyBatis, SpringBoot) p6spy/p6spy#561

Open

ayushtkn reviewed Dec 10, 2024

View reviewed changes

HIVE-15540: Impl DatabaseMetaData#getURL() and `DatabaseMetaData#ge…

edf9bda

…tUserName()` for HiveServer2 JDBC Driver

linghengqian force-pushed the presto-bump branch from 9f2a3e2 to edf9bda Compare December 12, 2024 08:18

asf-ci-hive added tests pending and removed tests passed labels Dec 12, 2024

asf-ci-hive added tests passed and removed tests pending labels Dec 12, 2024

ayushtkn approved these changes Dec 12, 2024

View reviewed changes

ayushtkn merged commit 28baba2 into apache:master Dec 12, 2024
6 checks passed

linghengqian deleted the presto-bump branch December 12, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-15540: Impl `DatabaseMetaData#getURL()` and `DatabaseMetaData#getUserName()` for HiveServer2 JDBC Driver #5554

HIVE-15540: Impl `DatabaseMetaData#getURL()` and `DatabaseMetaData#getUserName()` for HiveServer2 JDBC Driver #5554

linghengqian commented Nov 22, 2024 •

edited

Loading

InvisibleProgrammer commented Nov 22, 2024

InvisibleProgrammer Nov 22, 2024

linghengqian Nov 22, 2024

InvisibleProgrammer Nov 22, 2024 •

edited

Loading

linghengqian Nov 22, 2024

InvisibleProgrammer left a comment

linghengqian left a comment •

edited

Loading

ayushtkn left a comment

ayushtkn Dec 10, 2024

linghengqian Dec 12, 2024

ayushtkn Dec 10, 2024

linghengqian Dec 12, 2024

sonarqubecloud bot commented Dec 12, 2024

ayushtkn left a comment

HIVE-15540: Impl DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() for HiveServer2 JDBC Driver #5554

HIVE-15540: Impl DatabaseMetaData#getURL() and DatabaseMetaData#getUserName() for HiveServer2 JDBC Driver #5554

Conversation

linghengqian commented Nov 22, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

InvisibleProgrammer commented Nov 22, 2024

InvisibleProgrammer Nov 22, 2024

Choose a reason for hiding this comment

linghengqian Nov 22, 2024

Choose a reason for hiding this comment

InvisibleProgrammer Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

linghengqian Nov 22, 2024

Choose a reason for hiding this comment

InvisibleProgrammer left a comment

Choose a reason for hiding this comment

linghengqian left a comment • edited Loading

Choose a reason for hiding this comment

ayushtkn left a comment

Choose a reason for hiding this comment

ayushtkn Dec 10, 2024

Choose a reason for hiding this comment

linghengqian Dec 12, 2024

Choose a reason for hiding this comment

ayushtkn Dec 10, 2024

Choose a reason for hiding this comment

linghengqian Dec 12, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 12, 2024

Quality Gate passed

ayushtkn left a comment

Choose a reason for hiding this comment

HIVE-15540: Impl `DatabaseMetaData#getURL()` and `DatabaseMetaData#getUserName()` for HiveServer2 JDBC Driver #5554

HIVE-15540: Impl `DatabaseMetaData#getURL()` and `DatabaseMetaData#getUserName()` for HiveServer2 JDBC Driver #5554

linghengqian commented Nov 22, 2024 •

edited

Loading

InvisibleProgrammer Nov 22, 2024 •

edited

Loading

linghengqian left a comment •

edited

Loading