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: adds getValue to ResultSet #1073
feat: adds getValue to ResultSet #1073
Conversation
Adds a generic getValue method to the result set. It can be used to retrieve any type from the database.
Clirr does not support java 8 default implementations, so it thinks this are breaking changes. These in fact are not breaking changes, since we provide implementations in the interfaces.
@@ -592,4 +592,17 @@ | |||
<className>com/google/cloud/spanner/AsyncTransactionManager$CommitTimestampFuture</className> | |||
<method>java.lang.Object get()</method> | |||
</difference> | |||
|
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.
Clirr does not support Java 8 features, so it thinks these are breaking changes, where in fact we have provided default implementations for the added interface methods. No need for a major release here.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
Outdated
Show resolved
Hide resolved
Adds a less intrusive implementation of getValueInternal for the AbstractResultSet.
@olavloite got it, thanks for the feedback. I've re-implemented with a less intrusive manner, let me know if that makes sense. |
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void testReadNullInArrays() { |
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 be good to add null values for INT64
and FLOAT64
arrays as well in this test. Arrays of all types can always contain null values, regardless whether the column itself is nullable, and regardless of the array element 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.
These throw a NPE
as they do if you use a resultSet.getLongArray(...)
or resultSet.getDoubleArray(...)
. I could test the exception if you feel like we should.
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.
No, in that case I actually think we should change the implementation. It is logical that resultSet.getDoubleArray(..)
throws an NPE
. I don't think it is logical that resultSet.getValue("some-float64-array-col")
should ever throw an NPE
because one of the elements is null
, as a Value
can hold a null value for an element of an ARRAY<FLOAT64>
.
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.
Changed the implementation to support null
s in arrays and added tests for it.
In regards to a single null
value as in SELECT column FROM table
(where column
value is null
), when we do a getValue
it is currently throwing an exception as if we called a resultSet.getString(...)
. Do you think we should change this as well?
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.
Yes, I think we should change that as well now that we have a chance. Value
also has an isNull()
method, so it would make sense to let resultSet.getValue(...)
return a Value
instance when the column is null. The user can then check whether the value is actually null by calling Value#isNull()
.
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.
Awesome, changed the implementation to allow for that. If you could re-review, I'd appreciate it.
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 looks good to me, but I think for completeness it would be good to add null values to the integration tests for all array types. Now the primitive types are missing (Long and Double).
Uses the correct type for decoding structs from the result set
Makes it public the method to retrieve an array of structs from a Value
@@ -543,7 +543,7 @@ private Value() {} | |||
* | |||
* @throws IllegalStateException if {@code isNull()} or the value is not of the expected type | |||
*/ | |||
abstract List<Struct> getStructArray(); | |||
public abstract List<Struct> getStructArray(); |
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.
@olavloite I made this method public. It surprising me that this was not public before, since users can SELECT ARRAY(SELECT STRUCT ...)
and would not be able to get the results.
Codecov Report
@@ Coverage Diff @@
## master #1073 +/- ##
============================================
- Coverage 85.17% 84.93% -0.25%
- Complexity 2719 2727 +8
============================================
Files 155 156 +1
Lines 14351 14403 +52
Branches 1358 1380 +22
============================================
+ Hits 12224 12233 +9
- Misses 1558 1603 +45
+ Partials 569 567 -2 Continue to review full report at Codecov.
|
throw new UnsupportedOperationException("method should be overwritten"); | ||
} | ||
|
||
/** Returns the value of a non-{@code NULL} column as a {@link Value}. */ |
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.
nit: This should be updated to indicate that this method can be called for columns that contain a null value, and what the behavior is when the value is null.
Adds the method
getValue
to theResultSet
. This will return an encapsulated value. It supports all the column types that are provided.