-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-40249: [Java] Fix NPE in ArrowDatabaseMetadata #40988
Conversation
|
...t/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowDatabaseMetadata.java
Show resolved
Hide resolved
@@ -754,7 +754,11 @@ private <T> T getSqlInfoAndCacheIfCacheIsEmpty(final SqlInfo sqlInfoCommand, | |||
} | |||
|
|||
private String convertListSqlInfoToString(final List<?> sqlInfoList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make more sense to return Optional<String>
and update client code to explicitly decide what they want to do with missing values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is used by these methods:
- getSqlKeywords()
- getNumericFunctions()
- getStringFunctions()
- getSystemFunctions()
- getTimeDateFunctions()
These methods come from Avatica, so we can't easily change the return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can update our implementations to be explicit, right?
In any case, defaulting to an empty string also seems fine so long as it's documented in the helper's docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the return type of convertListSqlInfoToString()
to a Optional<String>
.
### Rationale for this change When retrieving database metadata using the JDBC driver, some data such as SQL keywords could be null. Before this change, an NPE would be thrown when trying to convert the list of SQL keywords into a String. ### What changes are included in this PR? The following database metadata fields: * SQL keywords * Numeric functions * String functions * System functions * Time/date functions will convert to an empty string when they are null. ### Are these changes tested? A unit test has been added to verify that the fields above are converted to the empty string when null, with no exceptions thrown. ### Are there any user-facing changes? The fields above will now return an empty string rather than throw an NPE.
35aa20a
to
2a51fa6
Compare
@github-actions crossbow submit java |
Revision: 2a51fa6 Submitted crossbow builds: ursacomputing/crossbow @ actions-f7fbc36bed |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 805327e. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change When retrieving database metadata using the JDBC driver, some data such as SQL keywords could be null. Before this change, an NPE would be thrown when trying to convert the list of SQL keywords into a String. ### What changes are included in this PR? The following database metadata fields: * SQL keywords * Numeric functions * String functions * System functions * Time/date functions will convert to an empty string when they are null. ### Are these changes tested? A unit test has been added to verify that the fields above are converted to the empty string when null, with no exceptions thrown. ### Are there any user-facing changes? The fields above will now return an empty string rather than throw an NPE. * GitHub Issue: apache#40249 Authored-by: Norman Jordan <[email protected]> Signed-off-by: David Li <[email protected]>
### Rationale for this change When retrieving database metadata using the JDBC driver, some data such as SQL keywords could be null. Before this change, an NPE would be thrown when trying to convert the list of SQL keywords into a String. ### What changes are included in this PR? The following database metadata fields: * SQL keywords * Numeric functions * String functions * System functions * Time/date functions will convert to an empty string when they are null. ### Are these changes tested? A unit test has been added to verify that the fields above are converted to the empty string when null, with no exceptions thrown. ### Are there any user-facing changes? The fields above will now return an empty string rather than throw an NPE. * GitHub Issue: apache#40249 Authored-by: Norman Jordan <[email protected]> Signed-off-by: David Li <[email protected]>
### Rationale for this change When retrieving database metadata using the JDBC driver, some data such as SQL keywords could be null. Before this change, an NPE would be thrown when trying to convert the list of SQL keywords into a String. ### What changes are included in this PR? The following database metadata fields: * SQL keywords * Numeric functions * String functions * System functions * Time/date functions will convert to an empty string when they are null. ### Are these changes tested? A unit test has been added to verify that the fields above are converted to the empty string when null, with no exceptions thrown. ### Are there any user-facing changes? The fields above will now return an empty string rather than throw an NPE. * GitHub Issue: apache#40249 Authored-by: Norman Jordan <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
When retrieving database metadata using the JDBC driver, some data such as SQL keywords could be null. Before this change, an NPE would be thrown when trying to convert the list of SQL keywords into a String.
What changes are included in this PR?
The following database metadata fields:
will convert to an empty string when they are null.
Are these changes tested?
A unit test has been added to verify that the fields above are converted to the empty string when null, with no exceptions thrown.
Are there any user-facing changes?
The fields above will now return an empty string rather than throw an NPE.