-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix findColumn
and get*(String label)
methods.
#185
Conversation
6e893ea
to
0e123bd
Compare
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.
Left some questions and suggestions 👍
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.
0e123bd
to
30350ea
Compare
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.
Added nit about optional but otherwise solid change,.
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 ? |
The ResultSet
findColumn
methods (BQForwardOnlyResultSet.findColumn
,BQScrollableResultSet.findColumn
, andBQResultSet.findColumn
) calledBQResultSetMetadata.getCatalogName
to determine the name of each column instead ofBQResultSetMetadata.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 aTYPE_SCROLL_INSENSITIVE
result set and thus get aBQScrollableResultSet
.BQScrollableResultSet
only accepts dates and times that can be parsed as longs, which disagrees with the more widely usedBQForwardOnlyResultSet
. Its date and time accessors are uncovered.getBigDecimal
is excluded because it callsBigDecimal.setScale
without a rounding mode, which no longer works in modern Java. A separate change can fix this.