-
Notifications
You must be signed in to change notification settings - Fork 114
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: add support for PG JSONB data type #1964
Conversation
Warning: This pull request is touching the following templated files:
|
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.
The PR looks good to me, there are a couple of minor nits, please take a look.
@@ -81,4 +81,25 @@ | |||
<className>com/google/cloud/spanner/connection/Connection</className> | |||
<method>com.google.spanner.v1.ResultSetStats analyzeUpdate(com.google.cloud.spanner.Statement, com.google.cloud.spanner.ReadContext$QueryAnalyzeMode)</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.
The clirr checks are not mandatory after #1931.
Just curious, Were you facing any specific errors?
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.
Ah, I did not know that. I'm so used to adding these that I run the clirr checks locally and then add what is needed. So yes; I did face errors, but only on my local machine :-)
@@ -200,14 +200,23 @@ public static Value string(@Nullable String v) { | |||
} | |||
|
|||
/** | |||
* Returns a {@code STRING} value. | |||
* Returns a {@code JSON} 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.
Thanks!
} | ||
|
||
@Override | ||
void valueToString(StringBuilder b) { |
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.
Sounds Good
@@ -85,6 +87,8 @@ public enum TypeAnnotationCode implements com.google.protobuf.ProtocolMessageEnu | |||
* <code>PG_NUMERIC = 2;</code> | |||
*/ | |||
public static final int PG_NUMERIC_VALUE = 2; | |||
/** <code>PG_JSONB = 3;</code> */ |
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: Looking at the comment above for PG_NUMERIC
, I'm wondering if we should add more details in comment for <code>PG_JSONB = 3;</code>
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.
That is probably a good idea, but this is in generated code, so this comment comes from the comments on the proto definition.
@@ -57,6 +57,8 @@ public enum TypeAnnotationCode implements com.google.protobuf.ProtocolMessageEnu | |||
* <code>PG_NUMERIC = 2;</code> | |||
*/ | |||
PG_NUMERIC(2), | |||
/** <code>PG_JSONB = 3;</code> */ |
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: Looking at the comment above for PG_NUMERIC
, I'm wondering if we should add more details in comment for <code>PG_JSONB = 3;</code>
|
||
@Test | ||
public void testPgJsonbInSecondaryIndex() { | ||
// JSONB is not allowed as a primary key. |
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: // JSONB is not allowed in index
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.
Thanks, updated
Type.newBuilder() | ||
.setCode(dialect == Dialect.POSTGRESQL ? TypeCode.STRING : TypeCode.JSON) | ||
.build(), | ||
dialect == Dialect.POSTGRESQL |
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.
Sounds Good
public void getPgJsonbList() { | ||
List<String> jsonList = new ArrayList<>(); | ||
jsonList.add("{\"color\":\"red\",\"value\":\"#f00\"}"); | ||
jsonList.add("{\"special\":\"%😃∮πρότερονแผ่นดินฮั่นเสื่อมሰማይᚻᛖ\"}"); |
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.
Sounds Good
import org.threeten.bp.Duration; | ||
|
||
// TODO: Re-enable when jsonb is GA. | ||
@Ignore("Feature is not yet generally available") |
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.
Ack
* chore: update dependency versions in java templates * update other templates Source-Link: googleapis/synthtool@0b86c72 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:68ba5f5164a4b55529d358bb262feaa000536a0c62980727dd05a91bbb47ea5e
* chore: update dependency versions in java templates * update other templates Source-Link: googleapis/synthtool@0b86c72 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:68ba5f5164a4b55529d358bb262feaa000536a0c62980727dd05a91bbb47ea5e Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Adds support for the PostgreSQL
jsonb
data type.