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
product-auto-label bot added the api: spanner 
google-cla bot added the cla: yes …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.
|
elharo left a comment
elharo left a comment There was a problem hiding this comment.
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.
why not just Preconditions and an IllegalArgumentException?
There was a problem hiding this comment.
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.
why not just assertEquals and assertTrue?
There was a problem hiding this comment.
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.
Could we create tests for the ARRAY, STRUCT and default cases?
There was a problem hiding this comment.
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.
JDBC arrays have an optional method that allow them to be converted to
ResultSets. 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 ofNUMERICcolumns, 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