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

Implement --proto-descriptor-file #195

Merged
merged 9 commits into from
Nov 6, 2024
Merged

Implement --proto-descriptor-file #195

merged 9 commits into from
Nov 6, 2024

Conversation

apstndb
Copy link
Collaborator

@apstndb apstndb commented Nov 5, 2024 •
edited
Loading

This PR implements --proto-descriptor-file option with USE statement compatibility.

$ go run ./ --database apstndb-sampledb3 --proto-descriptor-file order_descriptors.pb
Connected.
spanner> ALTER PROTO BUNDLE INSERT (`examples.shipping.OrderHistory`);
Query OK, 0 rows affected (11.11 sec)

spanner> USE apstndb-sampledb-with-data-idx;
spanner> ALTER PROTO BUNDLE INSERT (`examples.shipping.OrderHistory`);
Query OK, 0 rows affected (9.23 sec)

fixes #174

@apstndb apstndb changed the title Feature/proto descriptor file Implement --proto-descriptor-file Nov 5, 2024
@apstndb apstndb requested a review from yfuruyama November 5, 2024 21:33
yfuruyama
yfuruyama approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@yfuruyama yfuruyama left a comment

Choose a reason for hiding this comment

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

Thank you for filing this PR! I left some minor comments, but overall LGTM.

main.go Outdated
var protoDescriptorFileContent []byte
if opts.ProtoDescriptorFile != "" {
var err error
b, err := os.ReadFile(opts.ProtoDescriptorFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to use temporal variable b here as err is defined above.

protoDescriptorFileContent, err = os.ReadFile(opts.ProtoDescriptorFile)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you!
Fixed in 30e2eab

session.go Outdated
directedRead *pb.DirectedReadOptions
tc *transactionContext
tcMutex sync.Mutex // Guard a critical section for transaction.
protoDescriptorFileContent []byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe we can use more concise variable name like protoDescriptor because the session doesn't care if the proto descriptor was coming from file content or not.

Copy link
Collaborator Author

@apstndb apstndb Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!
Fixed in 30e2eab and 7a74cd5

yfuruyama
yfuruyama approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@yfuruyama yfuruyama left a comment

Choose a reason for hiding this comment

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

LGTM.

@yfuruyama yfuruyama merged commit ed6c09a into cloudspannerecosystem:master Nov 6, 2024
2 checks passed
@apstndb apstndb deleted the feature/proto-descriptor-file branch November 6, 2024 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yfuruyama yfuruyama yfuruyama approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Protocol Buffers
2 participants
@apstndb @yfuruyama