Skip to content

Conversation

olavloite
Copy link
Collaborator

@olavloite olavloite commented Aug 4, 2022

Adds support for the PostgreSQL jsonb data type.

@olavloite olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 4, 2022
@olavloite olavloite requested review from a team as code owners August 4, 2022 13:34
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Aug 4, 2022
@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/TypeProto.java
  • proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/type.proto

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Aug 4, 2022
@olavloite olavloite requested a review from ansh0l August 5, 2022 08:00
Copy link
Contributor

@ansh0l ansh0l left a 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>
Copy link
Contributor

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?

Copy link
Collaborator Author

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

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) {
Copy link
Contributor

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> */
Copy link
Contributor

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>

Copy link
Collaborator Author

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> */
Copy link
Contributor

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

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

Copy link
Collaborator Author

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

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\":\"%😃∮πρότερονแผ่นดินฮั่นเสื่อมሰማይᚻᛖ\"}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds Good

@olavloite olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 29, 2022
import org.threeten.bp.Duration;

// TODO: Re-enable when jsonb is GA.
@Ignore("Feature is not yet generally available")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ack

@olavloite olavloite merged commit d2b426f into main Aug 29, 2022
@olavloite olavloite deleted the jsonb branch August 29, 2022 17:38
gcf-owl-bot bot added a commit that referenced this pull request May 9, 2024
* 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
rahul2393 pushed a commit that referenced this pull request May 23, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants