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
feat: Support Array conversion to ResultSet #326
Conversation
JDBC arrays can optionally be converted to ResultSets. This feature was not implemented for the Cloud Spanner JDBC driver. This would cause DBeaver to show an error message instead of the actual data when fetching a NUMERIC array. Other arrays would be fetched correctly by DBeaver, as those array types do not use the conversion to a ResultSet.
…er-jdbc into array-to-resultset
…er-jdbc into array-to-resultset
…er-jdbc into array-to-resultset
Codecov Report
@@ Coverage Diff @@
## master #326 +/- ##
============================================
+ Coverage 71.35% 72.02% +0.67%
- Complexity 1114 1131 +17
============================================
Files 24 24
Lines 3474 3518 +44
Branches 531 537 +6
============================================
+ Hits 2479 2534 +55
+ Misses 778 765 -13
- Partials 217 219 +2
Continue to review full report at Codecov.
|
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.
Sounds reasonable, but could probably use an issue for tracking and release notes
public ResultSet getResultSet(long index, int count) throws SQLException { | ||
throw new SQLFeatureNotSupportedException(RESULTSET_NOT_SUPPORTED); | ||
public ResultSet getResultSet(long startIndex, int count) throws SQLException { | ||
JdbcPreconditions.checkArgument( |
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.
why not just Preconditions and an IllegalArgumentException?
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.
Because the JDBC specification requires the driver to throw an instance of SQLException
if the input is invalid. This applies to (almost) all methods defined by the JDBC specification, hence the separate JdbcPreconditions
helper class.
assertThat(array.getBaseType()).isEqualTo(Types.BOOLEAN); | ||
assertThat(((Boolean[]) array.getArray(1, 1))[0]).isEqualTo(Boolean.TRUE); | ||
try (ResultSet rs = array.getResultSet()) { | ||
assertThat(rs.next()).isTrue(); |
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.
why not just assertEquals and assertTrue?
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 library (and also the Java Spanner client) mostly use Truth for assertions, but some of the older classes use JUnit assertions. To reduce the mixing of the two, I tend to change the older classes to Truth when (larger) changes are made to them.
case TIMESTAMP: | ||
builder = binder.to(JdbcTypeConverter.toGoogleTimestamp((Timestamp) value)); | ||
break; | ||
case ARRAY: |
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.
Could we create tests for the ARRAY
, STRUCT
and default
cases?
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.
That's not really possible, because it is impossible to create an array with an invalid data type. It will fail already during creation, but I've added a couple of tests to verify that.
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.
LGTM
JDBC arrays have an optional method that allow them to be converted to
ResultSet
s. This method was not implemented for the Cloud Spanner JDBC driver. This would cause DBeaver to not be able to fetch and view the data ofNUMERIC
columns, as DBeaver would use this method for specifically those columns (other array columns would work). This PR fixes that specific problem and allows other users to use this convenience method for all array types.Fixes #332