feat: add support for Proto Columns#2779
Merged
harshachinta merged 32 commits intomainfrom Jan 26, 2024
Merged
Conversation
* 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
#2211) * samples: Adding samples for updating & querying Proto messages & enums * style: linting * style: linting * docs: Adding function and class doc
* 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
Co-authored-by: Pavol Juhos <pjuhos@google.com>
* 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
…to validate main branch update
|
Warning: This pull request is touching the following templated files:
|
…oogleapis/java-spanner into proto-column-enhancement-alpha
olavloite
approved these changes
Jan 25, 2024
Collaborator
olavloite
left a comment
There was a problem hiding this comment.
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
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
| v.forEach( | ||
| (message) -> { | ||
| if (message != null) { | ||
| serializedArray.add(ByteArray.copyFrom(message.toByteArray())); |
Collaborator
There was a problem hiding this comment.
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.
| "Proto message may not be null. Use MyProtoClass.getDefaultInstance() as a parameter value."); | ||
| checkNotNull(); | ||
| try { | ||
| return (T) m.toBuilder().mergeFrom(value.getByteArray().toByteArray()).build(); |
Collaborator
There was a problem hiding this comment.
Here also: Any possibility that we can do this conversion without copying to a byte array multiple times?
Contributor
Author
There was a problem hiding this comment.
@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())
| "Proto message may not be null. Use MyProtoClass.getDefaultInstance() as a parameter value."); | ||
| checkNotNull(); | ||
| try { | ||
| return (T) m.toBuilder().mergeFrom(value.toByteArray()).build(); |
Collaborator
There was a problem hiding this comment.
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
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Jan 29, 2024
🤖 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[DO NOT MERGE]
This PR has the changes from