Fix findColumn and get*(String label) methods.
#185
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goomrw
force-pushed
the
goomrw-get-by-label
branch
from
6e893ea
to
0e123bd
Compare
tanclary
reviewed
Dec 19, 2023
src/main/java/net/starschema/clouddb/jdbc/BQForwardOnlyResultSet.java
Outdated
Show resolved
Hide resolved
src/test/java/net/starschema/clouddb/jdbc/BQScrollableResultSetFunctionTest.java
Show resolved
Hide resolved
The ResultSet `findColumn` methods (`BQForwardOnlyResultSet.findColumn`, `BQScrollableResultSet.findColumn`, and `BQResultSet.findColumn`) called `BQResultSetMetadata.getCatalogName` to determine the name of each column instead of `BQResultSetMetadata.getColumnLabel`. This was wrong because `getCatalogName` always returns the BigQuery project ID, not the column name. This bug in turn broke the get methods that are implemented in terms of `findColumn`, e.g. `getString(String)`. This change fixes `findColumn` and adds tests for methods that use it with some caveats: - `BQResultSet` has its findColumn fixed but it's not tested. That's because it's unused, even by its tests, which request a `TYPE_SCROLL_INSENSITIVE` result set and thus get a `BQScrollableResultSet`. - `BQScrollableResultSet` only accepts dates and times that can be parsed as longs, which disagrees with the more widely used `BQForwardOnlyResultSet`. Its date and time accessors are uncovered. - `getBigDecimal` is excluded because it calls `BigDecimal.setScale` without a rounding mode, which no longer works in modern Java. A separate change can fix this.
Addresses code review comments.
goomrw
force-pushed
the
goomrw-get-by-label
branch
from
0e123bd
to
30350ea
Compare
goomrw
commented
Dec 19, 2023
fzakaria
reviewed
Dec 20, 2023
fzakaria
approved these changes
Dec 20, 2023
|
If we do much more work in this repo it might be a good idea to adopt some practices like:
These are some things Calcite does that I think work to its advantage, what do you think @goomrw @fzakaria ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ResultSet
findColumnmethods (BQForwardOnlyResultSet.findColumn,BQScrollableResultSet.findColumn, andBQResultSet.findColumn) calledBQResultSetMetadata.getCatalogNameto determine the name of each column instead ofBQResultSetMetadata.getColumnLabel.This was wrong because
getCatalogNamealways returns the BigQuery project ID, not the column name.This bug in turn broke the get methods that are implemented in terms of
findColumn, e.g.getString(String).This change fixes
findColumnand adds tests for methods that use it with some caveats:BQResultSethas its findColumn fixed but it's not tested. That's because it's unused, even by its tests, which request aTYPE_SCROLL_INSENSITIVEresult set and thus get aBQScrollableResultSet.BQScrollableResultSetonly accepts dates and times that can be parsed as longs, which disagrees with the more widely usedBQForwardOnlyResultSet. Its date and time accessors are uncovered.getBigDecimalis excluded because it callsBigDecimal.setScalewithout a rounding mode, which no longer works in modern Java. A separate change can fix this.