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(spanner): add support for Array of Proto columns #7014

Conversation

harshachinta
Copy link
Contributor

@harshachinta harshachinta commented Nov 10, 2022

This PR contains implementation changes to add support for handling array of Proto Messages and Proto Enum type columns.

To run integration tests, set the below environment variables:

GCLOUD_TESTS_GOLANG_PROJECT_ID=span-cloud-testing
GCLOUD_TESTS_GOLANG_SPANNER_HOST=staging-wrenchworks.sandbox.googleapis.com:443

@harshachinta harshachinta requested review from a team as code owners November 10, 2022 05:31
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the Spanner API. labels Nov 10, 2022
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.

Was the previous conversations lost?

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.

Generally LGTM, especially the test case. I will left the code style for the go client library experts, please double check with them. Thanks!

spanner/integration_test.go Outdated Show resolved Hide resolved
spanner/integration_test.go Show resolved Hide resolved
spanner/integration_test.go Show resolved Hide resolved
spanner/integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gauravpurohit06 gauravpurohit06 left a comment

Choose a reason for hiding this comment

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

Approve with minor comments.

}
etyp := typ.Elem().Elem()
switch acode {
case sppb.TypeCode_PROTO, sppb.TypeCode_BYTES:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please break it into smaller functions for better readability. One for PROTO/BYTES and other for ENUM/INT64.

sliceval := reflect.ValueOf(v)
etyp := reflect.TypeOf(v).Elem()

if etyp.Implements(protoMsgReflectType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.
Please break it into smaller functions for better readability, one for protoMsgReflectType and other for protoEnumReflectType.

spanner/integration_test.go Show resolved Hide resolved
spanner/integration_test.go Show resolved Hide resolved
spanner/integration_test.go Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
spanner/integration_test.go Show resolved Hide resolved
spanner/value.go Outdated Show resolved Hide resolved
@harshachinta harshachinta merged commit 9802570 into googleapis:proto-column-enhancement-alpha Nov 12, 2022
harshachinta added a commit that referenced this pull request May 7, 2024
* feat(spanner): add support for Proto Columns (#6886)

* feat(spanner): Adding support for Proto Message and Proto Enum

* feat(spanner): Add license header to new files

* code refactoring and additional checks

* nit: code changes

* go.mod update to prevent failing builds

* go.mod update to prevent failing builds

* go.mod and build_samples.sh update to prevent failing builds

* revert back grpc version

* revert back changes

* Using standard Singers example, refactoring redundant code

* code and proto refactoring

* Add proto_type_fqn for Proto Message and Proto Enum

* code refactoring

* go mod tidy: go.mod and go.sum version updates

* add changes to support compatibility between Int64 and Enum & compatibility between Bytes and Proto

* Revert "go mod tidy: go.mod and go.sum version updates"

This reverts commit 484b00c.

* add Integration Tests for Proto Message, Proto Enum, compatibility tests

* code refactoring

* code refactoring

* add unit tests for nil proto types

* Add error when nil proto message or nil enum is passed

* feat(spanner): add support for handling null value in Proto columns (#6954)

* feat(spanner): add support for handling null value in Proto columns

* code refactor: get protoTypeFqn from user defined nil types

* code refactoring

* code refactoring

* code refactoring

* code refactoring

* Add tests for MarshalJSON and UnmarshalJSON methods

* refactoring test file

* code refactoring

* feat(spanner): add support for Array of Proto columns (#7014)

* feat(spanner): add support for array of proto columns

* refactoring comments and added negative test cases while reading array of protos

* change decoding logic of handling array of proto columns

* feat(spanner): add support for handling null values in array of proto columns (#7042)

* feat(spanner): add support for handling null values in Array of Proto Columns

* add comments for code readability

* nit: change in error message

* feat(spanner): Modify configuration for integration test and add license header (#7059)

* feat(spanner): update go-genproto dependency (#7066)

* feat(spanner): support for enum columns as keys, index and integration tests (#7091)

* feat(spanner): support for proto columns as primary key and tests for parameterized queries, primary key and indexes

* feat(spanner): close read-only transaction to prevent session leak

* feat(spanner): update table schema to have GPK on proto field

* feat(spanner): add proto changes to support proto columns feature

* feat(spanner): remove gen-proto dep overwrite for proto column support

* feat(spanner): remove gen-proto dep overwrite from kokoro build

* feat(spanner): reset array of proto, enum when null from database (#7176)

* feat(spanner): reset array of proto, enum when null from database

* feat(spanner): fix license header to fix vet.sh build

* feat(spanner): fix proto generated file to fix vet.sh build

* feat(spanner): organize imports to fix vet.sh build

* feat(spanner): golint changes to fix vet.sh build

* feat(spanner): add support for Proto column DDL (#7292)

* feat(spanner): add proto changes for Proto Columns DDL support

* feat(spanner): add Proto descriptor file and integration tests for Proto DDL feature

* feat(spanner): skip pg tests and code refactoring for proto ddl

* feat(spanner): rename NewDatabaseAdminRESTClient to NewDatabaseAdminClient due to visibility label issue fix

* feat(spanner): remove hardcoded cloud-devel host

* feat(spanner): remove hardcoded project id

* feat(spanner): revert autogenerated code

* feat(spanner): just to validate integration tests running

* feat(spanner): change copyright year

* feat(spanner): set project id for integration tests

* feat(spanner): use jsoniter instead of json for marshal and unmarshal

* feat(spanner): revert presubmit.sh changes

* chore(spanner): run integration tests in presubmit

* chore(spanner): revert presubmit.sh changes

* feat(spanner): fix json

---------

Co-authored-by: rahul2393 <[email protected]>
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 Spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants