You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "dengzhhu653 (via GitHub)" <gi...@apache.org> on 2023/07/10 07:03:13 UTC
[GitHub] [hive] dengzhhu653 opened a new pull request, #4475: HIVE-27487: NPE in Hive JDBC storage handler
dengzhhu653 opened a new pull request, #4475:
URL: https://github.com/apache/hive/pull/4475
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
When deserializing the row read from Hive JDBC storage handler, in case if the column names of the row are full qualified, then it's unable to find the matching value from the row by the registered column, npe would happen:
https://github.com/apache/hive/blob/5e46e80bc7d059093aece81e3886ba5ee425ee95/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/JdbcSerDe.java#L228
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
No
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### Is the change a dependency upgrade?
No
<!--
If yes, please attach a file with output from mvn dependency:tree to validate a complete upgrade of dependency.
If no, write 'No'.
-->
### How was this patch tested?
Able to run the query against a table based on hive jdbc storage handler in the testing env.
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1667451264
LGTM, +1, minor comment can we dealt in a separate patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "nrg4878 (via GitHub)" <gi...@apache.org>.
nrg4878 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1283940834
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
@akshat0395 Does this explanation answer your question? limit 1 returns a row but we are just looking for row metadata from the result set.
This method is being overridden in this class because we want to use a limit 0 vs limit 1 that the parent class uses. Are you suggesting we change the GenericJdbcDatabaseAccessor.addLimitToQuery() ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1657587928
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1267146028
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
Cant we just call addLimitToQuery(sql, 0) directly? Why add another method for it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1668104378
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [11 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
 The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1283961020
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
@dengzhhu653 @nrg4878 Thanks for bringing the attention back on this, apologies for delay got caught up with other things.
Yes, the suggestion is to change GenericJdbcDatabaseAccessor.addLimitToQuery() and have a direct call for addLimitToQuery(sql, 0) in the base method itself. As you mentioned it only metadata and doesn't require limit 1.
I can understand why the current approach might have been take, in class `JethroDatabaseAccessor` we have done the same implementation of overriding the method and calling addLimitToQuery(sql, 0).
@dengzhhu653 are there any specific cases/sources where we would need limit 1 rather than limit 0 to get the metadata query?
If not, we should do this change at base itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1263227449
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java:
##########
@@ -108,59 +143,34 @@ private <T> List<T> getColumnMetadata(Configuration conf, ColumnMetadataAccessor
}
+ private <T> List<T> getColumnMetadata(ResultSet rs, ColumnMetadataAccessor<T> colAccessor)
Review Comment:
There are some places need to extract the result metadata from ResultSet,
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java#L69-L70
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L135
Method is declared as private, the upper caller will deal with the specific exception. For example:
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L250-L254
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java#L81-L84
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1262719536
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
wont limit 0 return an empty set? why is this needed?
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java:
##########
@@ -108,59 +143,34 @@ private <T> List<T> getColumnMetadata(Configuration conf, ColumnMetadataAccessor
}
+ private <T> List<T> getColumnMetadata(ResultSet rs, ColumnMetadataAccessor<T> colAccessor)
Review Comment:
Do we really need this method? we already have getColumnMetadata. I like the idea of making methods smaller and more comprehensive, in this case I feel we dont really need this method.
This also introduces a need to throw a generic exception instead a specific one which should be avoided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1285566948
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
For sake of simplicity for now, we can go ahead with the current changes, as you have already tested Mysql and postgres I don't think other datasources would have issues with limit 0 as well.
We can create a separate patch for refactor post some more testing around this and raise a patch then.
If that works for you please resolve the comment and move forward with the changes.
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 merged pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 merged PR #4475:
URL: https://github.com/apache/hive/pull/4475
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1628669529
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [11 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1630025700
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [11 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1263230160
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
The override method only cares about the query's result metadata,
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L128
An empty result set is enough for this purpose, no need to execute the whole query.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1283961020
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
@dengzhhu653 @nrg4878 Thanks for bringing the attention back on this, apologies for delay got caught up with other things.
Yes, the suggestion is to change GenericJdbcDatabaseAccessor.getMetaDataQuery() and have a direct call for addLimitToQuery(sql, 0) in the base method itself. As you mentioned it only metadata and doesn't require limit 1.
I can understand why the current approach might have been take, in class `JethroDatabaseAccessor` we have done the same implementation of overriding the method and calling addLimitToQuery(sql, 0).
@dengzhhu653 are there any specific cases/sources where we would need limit 1 rather than limit 0 to get the metadata query?
If not, we should do this change at base itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1668805112
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
 The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1664845624
Hi @nrg4878 @saihemanth-cloudera @akshat0395 can you please take a look if have cycles? I have tested the fix on my local HiveServer2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "nrg4878 (via GitHub)" <gi...@apache.org>.
nrg4878 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1283946417
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java:
##########
@@ -52,7 +51,8 @@ public class JdbcRecordIterator implements Iterator<Map<String, Object>> {
private String[] hiveColumnNames;
List<TypeInfo> hiveColumnTypesList;
- public JdbcRecordIterator(Connection conn, PreparedStatement ps, ResultSet rs, Configuration conf) throws HiveJdbcDatabaseAccessException {
+ public JdbcRecordIterator(GenericJdbcDatabaseAccessor accessor, Connection conn,
Review Comment:
General comment, not something this patch introduced. I wish these Connection/PreparedStatement/ResultSet objects were not being passed in but instead were accessible thru the accessor object. Makes a bit more sense IMHO. This code can just call into the accessor object for cleanup as well instead of doing it here.
What do you think? (I am not suggesting that we do this in this patch)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1267142971
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java:
##########
@@ -108,59 +143,34 @@ private <T> List<T> getColumnMetadata(Configuration conf, ColumnMetadataAccessor
}
+ private <T> List<T> getColumnMetadata(ResultSet rs, ColumnMetadataAccessor<T> colAccessor)
Review Comment:
Makes sense thanks for the explaination
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1268984558
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
This is where the method `getMetaDataQuery` takes place:
https://github.com/apache/hive/blob/master/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L74-L109
`GenericJdbcDatabaseAccessor` is the parent class of all jdbc sources. In this example, `HiveDatabaseAccessor` is a child of `GenericJdbcDatabaseAccessor` and override some methods the `GenericJdbcDatabaseAccessor` provided, `getMetaDataQuery` is one of the such method.
I'm not sure how to add `addLimitToQuery(sql, 0)` directly unless we change this line:
https://github.com/apache/hive/blob/master/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L86
but we couldn't assume the sql after `addLimitToQuery(sql, 0)` would return the correct column metadata on different kinds of sources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1667245535
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
 The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1668932134
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
 The version of Java (11.0.8) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more [here](https://docs.sonarcloud.io/appendices/scanner-environment/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1285360251
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/JdbcRecordIterator.java:
##########
@@ -52,7 +51,8 @@ public class JdbcRecordIterator implements Iterator<Map<String, Object>> {
private String[] hiveColumnNames;
List<TypeInfo> hiveColumnTypesList;
- public JdbcRecordIterator(Connection conn, PreparedStatement ps, ResultSet rs, Configuration conf) throws HiveJdbcDatabaseAccessException {
+ public JdbcRecordIterator(GenericJdbcDatabaseAccessor accessor, Connection conn,
Review Comment:
We get the Connection/PreparedStatement and release them on demand in accessor, it would be not easy to manage the lifecycle of them in accessor.
I'm trying to make DatabaseAccessor extend AutoCloseable, so that we can release the `DataSource` when necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] akshat0395 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "akshat0395 (via GitHub)" <gi...@apache.org>.
akshat0395 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1268947976
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
If my understanding is correct, this method only returns the query string, rather than actual result set. This method just relays the output string from another method, which seems like an extra method call to get the Query string.
calling `addLimitToQuery(sql, 0)` directly also should work fine in this case.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1287051894
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
Thanks @nrg4878 and @akshat0395, created HIVE-27583 for this discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1285362477
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
I tried on MySQL and Postgres, `limit 0` works for them. I wasn't applying the change in the base mainly because:
1. I couldn't figure out why it was on the original fix;
2. `limit 1` wouldn't introduce much overhead on these databases except Hive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] nrg4878 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "nrg4878 (via GitHub)" <gi...@apache.org>.
nrg4878 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1286981612
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
@akshat0395 Fair point about considering the change to limit 0 in the base code for all accessors. It think it would make sense if we are not doing anything with the first row of data. I also concur that it probably only adds significant overhead for hive.
I am good with the plan of forking it off into another jira. I would also like some additional work in confirming that we are pushing down the predicates to these DBs.
@dengzhhu653 Can you create a follow up jira and merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4475:
URL: https://github.com/apache/hive/pull/4475#issuecomment-1634778361
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4475)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4475&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4475&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4475&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a diff in pull request #4475: HIVE-27487: NPE in Hive JDBC storage handler
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #4475:
URL: https://github.com/apache/hive/pull/4475#discussion_r1267505725
##########
jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/HiveDatabaseAccessor.java:
##########
@@ -56,4 +57,19 @@ protected String addLimitToQuery(String sql, int limit) {
}
return sql + " LIMIT " + limit;
}
+
+ @Override
+ protected List<String> getColNamesFromRS(ResultSet rs) throws Exception {
+ List<String> columnNames = super.getColNamesFromRS(rs);
+ return columnNames.stream()
+ .map(c -> {
+ int lastIndex = c.lastIndexOf(".");
+ return lastIndex == -1 ? c : c.substring(lastIndex + 1); })
+ .collect(Collectors.toList());
+ }
+
+ @Override
+ protected String getMetaDataQuery(String sql) {
+ return addLimitToQuery(sql, 0);
Review Comment:
The default implement of getMetaDataQuery is actually adding `limit 1` to the`sql`,
https://github.com/apache/hive/blob/a838ca2760799708c465051904b96cab4ae628d1/jdbc-handler/src/main/java/org/apache/hive/storage/jdbc/dao/GenericJdbcDatabaseAccessor.java#L176-L178
I guess in some db vendors, `limit 0` will get an empty metadata of the result, that's why we cannot call `addLimitToQuery(sql, 0)` directly on all kinds of dbs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org