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: Support for Proto Messages & Enums #2155

Conversation

gauravpurohit06
Copy link
Contributor

@gauravpurohit06 gauravpurohit06 commented Nov 9, 2022

To add support for Proto Columns which includes messages and enums.

@gauravpurohit06 gauravpurohit06 requested review from a team as code owners November 9, 2022 06:48
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/java-spanner API. labels Nov 9, 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/Type.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/TypeCode.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/TypeOrBuilder.java
  • 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

@gauravpurohit06 gauravpurohit06 changed the title Support for Proto Messages & Enums feat: Support for Proto Messages & Enums Nov 9, 2022
@gauravpurohit06 gauravpurohit06 added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 9, 2022
@ansh0l ansh0l marked this pull request as draft November 9, 2022 09:02
@ansh0l ansh0l added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 9, 2022
@ansh0l ansh0l marked this pull request as ready for review November 28, 2022 17:34
Copy link
Member

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

Left a few comments, this is a long PR, will review changes to main and tests for spanner directory seprately

// If [code][] == [PROTO][TypeCode.PROTO] or [code][] ==
// [ENUM][TypeCode.ENUM], then `proto_type_fqn` is the fully qualified name of
// the proto type representing the proto/enum definition.
string proto_type_fqn = 5;
Copy link
Member

Choose a reason for hiding this comment

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

ack

@@ -145,6 +150,13 @@ enum TypeCode {
// preserved.
// - JSON array elements will have their order preserved.
JSON = 11;

Copy link
Member

Choose a reason for hiding this comment

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

ack

@@ -192,6 +192,46 @@
<className>com/google/cloud/spanner/StructReader</className>
<method>java.util.List getPgJsonbList(java.lang.String)</method>
</difference>
<difference>
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, the check is no longer required, but it is still active. Which means that if you don't add this, the build of the PR will be marked red (and some of us have brains that are not able to handle that, and need a green checkmark to be able to sleep at night ;-) )

Copy link

@Shua1 Shua1 left a comment

Choose a reason for hiding this comment

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

Please ask Sri Harsha to take a look at the test coverage and see if the coverage is comparable to the go client implementation. Thanks!

protected <T extends ProtocolMessageEnum> List<T> getProtoEnumListInternal(
int columnIndex, Function<Integer, ProtocolMessageEnum> method) {
Preconditions.checkNotNull(
method, "Method may not be null. Use 'MyProtoEnum::forNumber' as a parameter value.");
Copy link

Choose a reason for hiding this comment

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

Is it possible to call it for the user in Java?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Shua1: I prefer getProtoEnumListInternal(idx, my_method) here. Adding Function<Integer, ProtocolMessageEnum> method as a parameter makes more sense to me, because implementation for forNumber will be specific to the ProtocolMessageEnum class.

With getProtoEnumListInternal(idx), the Java client will not have any way to convert Long from Spanner backend into corresponding enum.

-Modified Docs/Comments
-Minor Refactoring
@gauravpurohit06 gauravpurohit06 requested review from olavloite, Shua1 and rajatbhatta and removed request for Shua1 December 13, 2022 06:55
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM, with a couple of minor nits.

Just to be certain: This cannot be merged as is. It needs to be rebased once the generated changes have been merged.

-code review comments
-adding function docs
@gauravpurohit06
Copy link
Contributor Author

gauravpurohit06 commented Dec 27, 2022

Just to be certain: This cannot be merged as is. It needs to be rebased once the generated changes have been merged.

Yes @olavloite , for now the changes will be merged in proto-column-enhancement-alpha branch and will only be merged in main once the generated changes have been merged.

@ansh0l ansh0l requested review from harshachinta and ansh0l and removed request for Shua1 December 28, 2022 10:41
@gauravpurohit06 gauravpurohit06 merged commit cbabd6c into googleapis:proto-column-enhancement-alpha Dec 28, 2022
harshachinta added a commit that referenced this pull request Jan 26, 2024
* feat: Support for Proto Messages & Enums (#2155)

* feat: Importing Proto Changes

Commit will be reverted, once PROTO changes are available publicly.

* feat: Proto Message Implementation

* feat: Adding support for enum

* feat: Code refactoring

Adding default implementation for newly added methods
ByteArray compatability changes for Proto Messages

* docs: Adding Java docs for all the newly added methods.

* test: Sample Proto & Generated classes for unit test

* feat: Adding bytes/proto & int64/enum compatability

Adding Additional check for ChecksumResultSet

* test: Adding unit tests

* test: Adding unit tests for ValueBinder.java

* feat: refactoring to add support for getValue & other minor changes

* feat: Minor refactoring

1. Adding docs and formatting the code.
2. Adding additional methods for enum and message which accepts descriptors.

* feat: Adding bytes/message & int64/enum compatability in Value

* refactor: Minor refactoring

* feat: Adding Proto Array Implementation

* test: Implementing unit tests for array of protos and enums

* refactor: adding clirr ignores

* feat: Adding support for enum as Primary Key

* feat: Code Review Changes, minor refactoring and adding docs

* feat: Addressing review comments

-Modified Docs/Comments
-Minor Refactoring

* refactor: Using Column instead of column to avoid test failures

* feat: Minor refactoring

-code review comments
-adding function docs

* samples: Adding samples for updating & querying Proto messages & enums (#2211)

* samples: Adding samples for updating & querying Proto messages & enums

* style: linting

* style: linting

* docs: Adding function and class doc

* test: Proto Column Integration tests (#2212)

* test: Adding Integration tests for Proto Messages & Enums

* test: Adding additional test for Parameterized Queries, Primary Keys & Invalid Wire type errors.

* style: Formatting

* style: Formatting

* test: Updating instance and db name

* test: Adding inter compatability check while writing data

* Configured jitpack.yml to use OpenJDK 11 (#2218)

Co-authored-by: Pavol Juhos <pjuhos@google.com>

* feat: add support for Proto Columns DDL (#2277)

* feat: add code changes and tests for Proto columns DDL support

* feat: add auto generated code

* feat: code changes and tests for Proto columns DDL support

* feat: add descriptors file

* feat: code refactoring

* feat: Integration tests and code refactoring

* feat: code refactoring

* feat: unit tests and clirr differences

* feat: lint changes

* feat: code refactor

* feat: code refactoring

* feat: code refactoring

* feat: code refactoring

* feat: add java docs to new methods

* feat: lint formatting

* feat: lint formatting changes

* feat: lint formatting

* feat: lint formatting

* feat: test exception cases

* feat: code refactoring

* feat: add java docs and refactoring

* feat: add java docs

* feat: java docs refactor

* feat: remove overload method setProtoDescriptors that accepts file path as input to avoid unexpected issues

* feat: remove updateDdl method overload to update proto descriptor

* teat: update pom file to run tests on cloud-devel region temporarily to validate main branch update

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* fix: revert host changes in pom.xml file

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat(spanner): revert autogenerated code

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat(spanner): remove samples

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat(spanner): remove clirr

* feat(spanner): skip emulator test

* feat(spanner): clirr

* feat(spanner): fix javadoc

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat(spanner): fix javadoc

* feat(spanner): fix javadoc

* feat(spanner): fix javadoc

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat(spanner): fix emulator skip

* build: ignore all changes in v1 package

* feat(spanner): add optimizations to deserialize proto messages

* feat(spanner): remove TODO

* feat(spanner): remove TODO

---------

Co-authored-by: Gaurav Purohit <gauravpurohit@google.com>
Co-authored-by: Pavol Juhos <pjuhos@google.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.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. do not merge Indicates a pull request not ready for merge, due to either quality or timing. kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants