Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Spanner - add NUMERIC type #2518

Merged
merged 5 commits into from
Sep 16, 2020
Merged

Spanner - add NUMERIC type #2518

merged 5 commits into from
Sep 16, 2020

Conversation

dmitry-s
Copy link
Contributor

fixes #2515

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall!

Can you also update the sample where we use Double price to use BigDecimal price?

Looks like there is a compilation error in Firestore module now. Probably has to do with dep version changes.

@@ -91,6 +94,7 @@
.put(double[].class, AbstractStructReader::getDoubleArray)
.put(long[].class, AbstractStructReader::getLongArray)
.put(boolean[].class, AbstractStructReader::getBooleanArray)
.put(BigDecimal.class, AbstractStructReader::getBigDecimal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a duplicate of the above map, but there's probably some obscure reason for it. :)

+ "FROM trades WHERE ( LOWER(action)=LOWER(@tag0) "
+ "AND ticker=@tag1 ) OR "
+ "( trader_id=@tag2 AND price<@tag3 ) OR ( price>=@tag4 AND id<>NULL AND "
+ "trader_id=NULL AND trader_id LIKE @tag7 AND price=TRUE AND price=FALSE AND "
+ "price>@tag10 AND price<=@tag11 AND price IN UNNEST(@tag12) ) ORDER BY id DESC LIMIT 3";
+ "price>@tag10 AND price<=@tag11 AND price IN UNNEST(@tag12) "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

price is actually a perfect field to use BigDecimal for.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of optional nits, LGTM.

@@ -91,6 +94,7 @@
.put(double[].class, AbstractStructReader::getDoubleArray)
.put(long[].class, AbstractStructReader::getLongArray)
.put(boolean[].class, AbstractStructReader::getBooleanArray)
.put(BigDecimal.class, AbstractStructReader::getBigDecimal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit -- should this go before the primitive arrays?

@@ -71,6 +73,7 @@
.put(double[].class, AbstractStructReader::getDoubleArray)
.put(long[].class, AbstractStructReader::getLongArray)
.put(boolean[].class, AbstractStructReader::getBooleanArray)
.put(BigDecimal.class, AbstractStructReader::getBigDecimal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should this go to before array types?

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #2518 into master will increase coverage by 74.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2518       +/-   ##
=============================================
+ Coverage      0.00%   74.25%   +74.25%     
- Complexity        0     2162     +2162     
=============================================
  Files           267      267               
  Lines          7755     7760        +5     
  Branches          0      803      +803     
=============================================
+ Hits              0     5762     +5762     
- Misses            0     1628     +1628     
- Partials          0      370      +370     
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 74.25% <100.00%> (?) 2162.00 <0.00> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...vert/ConverterAwareMappingSpannerEntityWriter.java 95.91% <100.00%> (+95.91%) 51.00 <0.00> (+51.00)
...p/data/spanner/core/convert/SpannerTypeMapper.java 100.00% <100.00%> (+100.00%) 7.00 <0.00> (+7.00)
.../gcp/data/spanner/core/convert/StructAccessor.java 92.10% <100.00%> (+92.10%) 12.00 <0.00> (+12.00)
...ramework/cloud/gcp/vision/DocumentOcrTemplate.java 17.64% <0.00%> (+17.64%) 4.00% <0.00%> (+4.00%)
...amework/cloud/gcp/vision/DocumentOcrResultSet.java 36.36% <0.00%> (+36.36%) 6.00% <0.00%> (+6.00%)
...ework/cloud/gcp/storage/GoogleStorageResource.java 81.96% <0.00%> (+81.96%) 41.00% <0.00%> (+41.00%)
...work/cloud/gcp/bigquery/core/BigQueryTemplate.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...mework/cloud/gcp/data/datastore/core/LazyUtil.java 79.54% <0.00%> (+79.54%) 10.00% <0.00%> (+10.00%)
...oud/gcp/storage/GoogleStorageProtocolResolver.java 75.00% <0.00%> (+75.00%) 10.00% <0.00%> (+10.00%)
...k/cloud/gcp/data/spanner/core/SpannerTemplate.java 77.39% <0.00%> (+77.39%) 89.00% <0.00%> (+89.00%)
... and 57 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 cc2a98f...e296093. Read the comment docs.

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for Spanner, but can we do the Firestore fix in a separate PR please?

@@ -27,7 +25,20 @@ private Util() {
}

public static String extractDatabasePath(String parent) {
return parent.substring(0, StringUtils.ordinalIndexOf(parent, "/", 4));
//the parent looks like this: projects/{project_id}/databases/{database_id}/...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated. Can we please put it in a separate PR?

@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@dmitry-s dmitry-s merged commit fda989f into master Sep 16, 2020
@dmitry-s dmitry-s deleted the spanner-numeric-type branch September 16, 2020 13:58
meltsufin pushed a commit that referenced this pull request Oct 8, 2020
* Spanner - add NUMERIC type; fixes #2515
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add NUMERIC data type support for Spanner (BigDecimal)
3 participants