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.

Sign up for GitHub

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

Jump to bottom

feat: add support for Proto Columns #2779

Merged
merged 32 commits into from Jan 26, 2024
Merged

feat: add support for Proto Columns #2779

merged 32 commits into from Jan 26, 2024

Conversation

harshachinta
Copy link
Contributor

@harshachinta harshachinta commented Jan 10, 2024 •
edited

gauravpurohit06 and others added 11 commits December 28, 2022 16:20
@gauravpurohit06
feat: Support for Proto Messages & Enums (#2155)
cbabd6c 
* 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
@gauravpurohit06
samples: Adding samples for updating & querying Proto messages & enums (
1fb6e94 
#2211)

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

* style: linting

* style: linting

* docs: Adding function and class doc
@gauravpurohit06
test: Proto Column Integration tests (#2212)
6c97b99 
* 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
@gauravpurohit06
Merge remote-tracking branch 'upstream/main' into proto-column-enhanc…
dd358c2 
…ement-alpha
@gauravpurohit06 @pjuhos
Configured jitpack.yml to use OpenJDK 11 (#2218)
53397e6 
Co-authored-by: Pavol Juhos <pjuhos@google.com>
@harshachinta
feat: add support for Proto Columns DDL (#2277)
2f8eb42 
* 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
@harshachinta
Merge branch 'main' into proto-column-enhancement-alpha
a70f259
@harshachinta
teat: update pom file to run tests on cloud-devel region temporarily …
9513753 
…to validate main branch update
@gcf-owl-bot
🦉 Updates from OwlBot post-processor
6701f52 
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
@harshachinta
fix: revert host changes in pom.xml file
f63da18
@harshachinta
Merge branch 'main' into proto-column-enhancement-alpha
5fbef25
@harshachinta harshachinta requested review from a team as code owners January 10, 2024 13:31
@generated-files-bot Generated Files Bot
Copy link

generated-files-bot bot commented Jan 10, 2024 •
edited

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

  • google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClient.java
  • google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClientTest.java

@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 Jan 10, 2024
@harshachinta harshachinta added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 10, 2024
olavloite
olavloite approved these changes Jan 25, 2024
View reviewed changes
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.

Looks generally good to me, but with some requests for optimizations. Feel free to defer those to a later PR if this is urgent to get merged.

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
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Type.java Outdated Show resolved Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java
v.forEach(
(message) -> {
if (message != null) {
serializedArray.add(ByteArray.copyFrom(message.toByteArray()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It would have been great if we could have done this conversion without copying the byte array twice (message.toByteArray() creates a new byte[], and the copyFrom method then makes a copy of that again).

I had a quick look, and could not find anything, so feel free to ignore.

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java Outdated
"Proto message may not be null. Use MyProtoClass.getDefaultInstance() as a parameter value.");
checkNotNull();
try {
return (T) m.toBuilder().mergeFrom(value.getByteArray().toByteArray()).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here also: Any possibility that we can do this conversion without copying to a byte array multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olavloite I have applied the optimization suggested above to this case too. Please let me know if this is correct.

value.getByteArray().toByteArray()

is changed to

Base64.getDecoder()
                .wrap(
                    CharSource.wrap(value.getBase64String())
                        .asByteSource(StandardCharsets.UTF_8)
                        .openStream())

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java
"Proto message may not be null. Use MyProtoClass.getDefaultInstance() as a parameter value.");
checkNotNull();
try {
return (T) m.toBuilder().mergeFrom(value.toByteArray()).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here also: Can we do a more direct conversion?

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Value.java Outdated Show resolved Hide resolved
...loud-spanner/src/test/java/com/google/cloud/spanner/connection/RandomResultSetGenerator.java Show resolved Hide resolved
jitpack.yml Outdated Show resolved Hide resolved
@harshachinta harshachinta removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 26, 2024
@harshachinta harshachinta merged commit 30d37dd into main Jan 26, 2024
27 checks passed
@harshachinta harshachinta deleted the proto-column-enhancement-alpha branch January 26, 2024 13:33
@release-please release-please bot mentioned this pull request Jan 26, 2024
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 29, 2024
@release-please
chore(main): release 6.57.0 (#2782)
32aace1 
🤖 I have created a release *beep* *boop*
---


## [6.57.0](https://togithub.com/googleapis/java-spanner/compare/v6.56.0...v6.57.0) (2024-01-29)


### Features

* Add FLOAT32 enum to TypeCode ([#2800](https://togithub.com/googleapis/java-spanner/issues/2800)) ([383fea5](https://togithub.com/googleapis/java-spanner/commit/383fea5b5dc434621585a1b5cfd128a01780472a))
* Add support for Proto Columns ([#2779](https://togithub.com/googleapis/java-spanner/issues/2779)) ([30d37dd](https://togithub.com/googleapis/java-spanner/commit/30d37dd80c91b2dffdfee732677607ce028fb8d2))
* **spanner:** Add proto descriptors for proto and enum types in create/update/get database ddl requests ([#2774](https://togithub.com/googleapis/java-spanner/issues/2774)) ([4a906bf](https://togithub.com/googleapis/java-spanner/commit/4a906bf2719c30dcd7371f497a8a28c250db77be))


### Bug Fixes

* Remove google-cloud-spanner-executor from the BOM ([#2844](https://togithub.com/googleapis/java-spanner/issues/2844)) ([655000a](https://togithub.com/googleapis/java-spanner/commit/655000a3b0471b279cbcbe8a4a601337e7274ef8))


### Dependencies

* Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.22.0 ([#2785](https://togithub.com/googleapis/java-spanner/issues/2785)) ([f689f74](https://togithub.com/googleapis/java-spanner/commit/f689f742d8754134523ed0394b9c1b8256adcae2))
* Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.23.0 ([#2801](https://togithub.com/googleapis/java-spanner/issues/2801)) ([95f064f](https://togithub.com/googleapis/java-spanner/commit/95f064f9f60a17de375e532ec6dd78dca0743e79))


### Documentation

* Samples and tests for instance APIs. ([#2768](https://togithub.com/googleapis/java-spanner/issues/2768)) ([88e24c7](https://togithub.com/googleapis/java-spanner/commit/88e24c7a7d046056605a2a824450e0153b339c86))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
@lqiu96 lqiu96 mentioned this pull request Feb 2, 2024
Open
@harshachinta harshachinta restored the proto-column-enhancement-alpha branch February 5, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: xl Pull request size is extra large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@harshachinta @olavloite @gauravpurohit06