feat(spanner): add support for Proto Columns in Connection API by harshachinta · Pull Request #3123 · googleapis/java-spanner (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits37 Checks26 Files changed

Conversation

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

[ Show hidden characters]({{ revealButtonHref }})

Adds support for Proto Columns feature in the Connection API.
This PR is taken from #2495.

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

Adding docs and formatting the code.
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

googleapis#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

… fix autogenerated client side statement tests

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

proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/GetDatabaseDdlResponse.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/SpannerDatabaseAdminProto.java
proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/UpdateDatabaseDdlRequest.java
proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto
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/TypeProto.java

harshachinta changed the title~~feat(spanner): add support for Proto Columns to Connection API~~ feat(spanner): add support for Proto Columns in Connection API

May 23, 2024

Comment on lines +1944 to +1946

// reset proto descriptors after executing a DDL statement
this.protoDescriptors = null;
this.protoDescriptorsFilePath = null;

Choose a reason for hiding this comment

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

Would it make more sense to move this to setDefaultTransactionOptions()? It keeps the code cleaner, as we have all the reset code in one method, but it does mean that executing for example a query after setting the proto descriptors also clears the fields. One option to prevent the latter could be to add an input argument to setDefaultTransactionOptions() that indicates the type of statement that was executed, and only reset these fields if the last statement was a DDL statement.

Choose a reason for hiding this comment

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

sure, will check if it makes sense to move that logic to setDefaultTransactionOptions() as a seperate PR. This PR is very big that the code review will be difficult. Will work on this immediately.

Choose a reason for hiding this comment

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

This comment will be addressed in PR #3126

private final String instanceId;
private final String databaseName;

static class Builder {
private DatabaseAdminClient dbAdminClient;
private String projectId;

Choose a reason for hiding this comment

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

nit: Instead of adding this field, could we change the individual fields projectId, instanceId, databaseName into one field of type DatabaseId?