Skip to content
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

Merged
merged 11 commits into from Apr 25, 2021

Conversation

thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Apr 19, 2021

Adds the method getValue to the ResultSet. This will return an encapsulated value. It supports all the column types that are provided.

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.
@thiagotnunes thiagotnunes requested review from as code owners Apr 19, 2021
@product-auto-label product-auto-label bot added the api: spanner label Apr 19, 2021
@google-cla google-cla bot added the cla: yes label Apr 19, 2021
@@ -592,4 +592,17 @@
<className>com/google/cloud/spanner/AsyncTransactionManager$CommitTimestampFuture</className>
<method>java.lang.Object get()</method>
</difference>

Copy link
Contributor Author

@thiagotnunes thiagotnunes Apr 19, 2021

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.

Adds a less intrusive implementation of getValueInternal for the
AbstractResultSet.
@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Apr 19, 2021

@olavloite got it, thanks for the feedback. I've re-implemented with a less intrusive manner, let me know if that makes sense.

@thiagotnunes thiagotnunes requested a review from olavloite Apr 19, 2021
}

@Test
public void testReadNullInArrays() {
Copy link
Contributor

@olavloite olavloite Apr 19, 2021

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.

Copy link
Contributor Author

@thiagotnunes thiagotnunes Apr 19, 2021

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.

Copy link
Contributor

@olavloite olavloite Apr 19, 2021

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>.

Copy link
Contributor Author

@thiagotnunes thiagotnunes Apr 20, 2021

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 nulls 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?

Copy link
Contributor

@olavloite olavloite Apr 20, 2021

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().

Copy link
Contributor Author

@thiagotnunes thiagotnunes Apr 21, 2021

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.

Copy link
Contributor

@olavloite olavloite left a comment

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();
Copy link
Contributor Author

@thiagotnunes thiagotnunes Apr 20, 2021

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
Copy link

@codecov codecov bot commented Apr 20, 2021

Codecov Report

Merging #1073 (cab531b) into master (1eeb6aa) will decrease coverage by 0.24%.
The diff coverage is 21.15%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ Complexity Δ
...va/com/google/cloud/spanner/AbstractResultSet.java 79.13% <0.00%> (-4.32%) 28.00 <0.00> (ø)
...m/google/cloud/spanner/ForwardingStructReader.java 24.27% <0.00%> (-0.99%) 12.00 <0.00> (ø)
...in/java/com/google/cloud/spanner/StructReader.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../src/main/java/com/google/cloud/spanner/Value.java 87.81% <ø> (ø) 63.00 <0.00> (ø)
...oud/spanner/connection/DirectExecuteResultSet.java 96.77% <50.00%> (-1.56%) 61.00 <2.00> (+2.00) ⬇️
...ner/connection/ReplaceableForwardingResultSet.java 96.72% <50.00%> (-1.59%) 58.00 <2.00> (+2.00) ⬇️
...com/google/cloud/spanner/AbstractStructReader.java 98.40% <80.00%> (-0.77%) 53.00 <2.00> (+2.00) ⬇️
...main/java/com/google/cloud/spanner/ResultSets.java 97.08% <100.00%> (+0.05%) 5.00 <0.00> (ø)
...src/main/java/com/google/cloud/spanner/Struct.java 88.78% <100.00%> (+0.10%) 28.00 <0.00> (ø)
.../google/cloud/spanner/AbstractLazyInitializer.java 92.85% <0.00%> (-7.15%) 4.00% <0.00%> (-1.00%)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eeb6aa...cab531b. Read the comment docs.

@thiagotnunes thiagotnunes requested a review from olavloite Apr 21, 2021
throw new UnsupportedOperationException("method should be overwritten");
}

/** Returns the value of a non-{@code NULL} column as a {@link Value}. */
Copy link
Contributor

@olavloite olavloite Apr 21, 2021

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.

@thiagotnunes thiagotnunes merged commit 7792c90 into googleapis:master Apr 25, 2021
15 of 17 checks passed
@thiagotnunes thiagotnunes deleted the result-set-get-value branch Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants