feat: adds getValue to ResultSet #1073

thiagotnunes · 2021-04-19T06:06:24Z

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 · 2021-04-19T06:07:30Z

google-cloud-spanner/clirr-ignored-differences.xml

@@ -592,4 +592,17 @@
    <className>com/google/cloud/spanner/AsyncTransactionManager$CommitTimestampFuture</className>
    <method>java.lang.Object get()</method>
  </difference>
+


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

Adds a less intrusive implementation of getValueInternal for the AbstractResultSet.

thiagotnunes · 2021-04-19T09:28:54Z

@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

olavloite · 2021-04-19T09:51:02Z

google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITResultSetGetValue.java

+  }
+
+  @Test
+  public void testReadNullInArrays() {


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.

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.

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

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?

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

Awesome, changed the implementation to allow for that. If you could re-review, I'd appreciate it.

olavloite

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

thiagotnunes · 2021-04-20T01:24:12Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java

@@ -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();


@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 · 2021-04-20T01:44:52Z

Codecov Report

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

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

olavloite · 2021-04-21T06:08:12Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/StructReader.java

+    throw new UnsupportedOperationException("method should be overwritten");
+  }
+
+  /** Returns the value of a non-{@code NULL} column as a {@link Value}. */


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 added 3 commits April 19, 2021 15:49

feat: adds getValue method to result set

a38c291

Adds a generic getValue method to the result set. It can be used to retrieve any type from the database.

feat: removes custom int64 and float64 arrays

88e8f35

feat: fixes clirr checks

6a5680c

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 requested review from a team as code owners April 19, 2021 06:06

product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Apr 19, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 19, 2021

thiagotnunes commented Apr 19, 2021

View reviewed changes

thiagotnunes requested review from olavloite, zoercai and skuruppu April 19, 2021 06:11

olavloite reviewed Apr 19, 2021

View reviewed changes

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

thiagotnunes added 2 commits April 19, 2021 16:51

test: fixes GrpcResultSet serialize test

7c2e990

feat: implements getValueInternal in result set

06d2e9c

Adds a less intrusive implementation of getValueInternal for the AbstractResultSet.

thiagotnunes requested a review from olavloite April 19, 2021 09:29

thiagotnunes commented Apr 19, 2021

View reviewed changes

google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractResultSet.java Show resolved Hide resolved

olavloite reviewed Apr 19, 2021

View reviewed changes

olavloite approved these changes Apr 19, 2021

View reviewed changes

thiagotnunes added 4 commits April 20, 2021 10:42

fix: fixes struct value decoding

3a76dc0

Uses the correct type for decoding structs from the result set

feat: makes the structArray method public

49f3d83

Makes it public the method to retrieve an array of structs from a Value

test: adds tests for value literals

02a027e

feat: accepts null in arrays for getValue

285b048

thiagotnunes commented Apr 20, 2021

View reviewed changes

feat: allows nulls in values from result sets

bab743f

thiagotnunes requested a review from olavloite April 21, 2021 01:43

olavloite approved these changes Apr 21, 2021

View reviewed changes

docs: fixes java doc for getValue

cab531b

thiagotnunes merged commit 7792c90 into googleapis:master Apr 25, 2021

thiagotnunes deleted the result-set-get-value branch April 25, 2021 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds getValue to ResultSet #1073

feat: adds getValue to ResultSet #1073

thiagotnunes commented Apr 19, 2021

thiagotnunes Apr 19, 2021

thiagotnunes commented Apr 19, 2021

olavloite Apr 19, 2021

thiagotnunes Apr 19, 2021

olavloite Apr 19, 2021 •

edited

thiagotnunes Apr 20, 2021

olavloite Apr 20, 2021

thiagotnunes Apr 21, 2021

olavloite left a comment

thiagotnunes Apr 20, 2021 •

edited

codecov bot commented Apr 20, 2021 •

edited

olavloite Apr 21, 2021

feat: adds getValue to ResultSet #1073

feat: adds getValue to ResultSet #1073

Conversation

thiagotnunes commented Apr 19, 2021

thiagotnunes Apr 19, 2021

Choose a reason for hiding this comment

thiagotnunes commented Apr 19, 2021

olavloite Apr 19, 2021

Choose a reason for hiding this comment

thiagotnunes Apr 19, 2021

Choose a reason for hiding this comment

olavloite Apr 19, 2021 • edited

Choose a reason for hiding this comment

thiagotnunes Apr 20, 2021

Choose a reason for hiding this comment

olavloite Apr 20, 2021

Choose a reason for hiding this comment

thiagotnunes Apr 21, 2021

Choose a reason for hiding this comment

olavloite left a comment

Choose a reason for hiding this comment

thiagotnunes Apr 20, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Apr 20, 2021 • edited

Codecov Report

olavloite Apr 21, 2021

Choose a reason for hiding this comment

olavloite Apr 19, 2021 •

edited

thiagotnunes Apr 20, 2021 •

edited

codecov bot commented Apr 20, 2021 •

edited