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.
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/spannertest): Support generated columns #4742
feat(spanner/spannertest): Support generated columns #4742
Changes from 13 commits
d70a2fe
1469ccd
8e160e7
d247e11
023d966
3babbb9
d56eb3b
af15328
f55835d
8f53cad
e3c82c2
2bf349c
1c9fe2c
03d80d3
d33754c
e04ef36
99a8c91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this
->This
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the missing period at the end
... inserted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have a convention to limit the line length of comments to 80 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This check should be extended to check whether the table is empty or not. Adding a generated column to an empty tables should be safe in this case.
This PR should also add a check to
dropColumn
to prevent a column that is a dependent of a generated column from being dropped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already checking the table is non empty, as for checking if its a dependency of a generated column do you still think its worth adding that as we talked about dropping all dependency calculations above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed that it was already checking it.
As for the dependency checking:
dropColumn
to prevent columns that are used by a generated column from being dropped. But preferably that dependency checking can be done using the method that also does the evaluation, so that we don't have to duplicate that code. Would it for example be possible to use default values for the evaluation context when we only want to check the dependencies?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that definitely sounds like the right approach, having a way to run the evaluator in with default values in some "complete" way (i.e. we'd have to check in an OR what is required to evaluate the RHS even if the LHS evaluates to true). It feels out of scope for me right now, given it will be a fair bit of extra work to get going well and I'm not overly confident with the evaluation code and typing. For now I'll just come up with the solution for adding columns as any other features should be able to be built on top without changes so doesn't feel wasteful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please well format the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add tests for:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Add error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add tests for update and delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change belongs in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can move it to a separate one, just smuggled it in here as I was testing with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. I would prefer to have it in a separate PR, unless it is actually used in one of the test cases (I don't think it is at this moment, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should be
Poor writer
, right?