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: create sequence backend #820

Merged

Conversation

asthamohta
Copy link
Collaborator

No description provided.

@asthamohta asthamohta requested a review from a team as a code owner May 8, 2024 16:20
@asthamohta asthamohta requested review from manitgupta and Deep1998 and removed request for a team May 8, 2024 16:20
Copy link
Member

@manitgupta manitgupta left a comment

Choose a reason for hiding this comment

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

Excellent PR! Love the rigor you've put in in the UTs 👍✌🏻

I don't have any major concerns, except -

Please validate backward compatibility of the session file. Can you find a way to write a test which uses an older version of the session file and the newer version (which contains sequences now) and ensure that both can be parsed and both work?

We should have a method which loads the session file which you can use to write this type of test.

spanner/ddl/ast.go Show resolved Hide resolved
@Deep1998
Copy link
Collaborator

Excellent PR! Love the rigor you've put in in the UTs 👍✌🏻

I don't have any major concerns, except -

Please validate backward compatibility of the session file. Can you find a way to write a test which uses an older version of the session file and the newer version (which contains sequences now) and ensure that both can be parsed and both work?

We should have a method which loads the session file which you can use to write this type of test.

Agreed, we shuold be able to add an IT here using the Spanner emulator similar to our existing ones.

@asthamohta
Copy link
Collaborator Author

@Deep1998 and @manitgupta, added unit tests reading session file into conv(new and old) to ensure an older version is also correctly read. Please check. Thanks

@manitgupta manitgupta merged commit 4af4747 into GoogleCloudPlatform:master May 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants