-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
opt: wrap virtual computed column projections in a cast #105736
opt: wrap virtual computed column projections in a cast #105736
Conversation
Hold off on reviewing - looks like this regressed some query plans. I'll see if there's a way to fix that. |
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @msirek)
-- commits
line 6 at r1:
Probably #73743.
This commit removes a TODO that was partially addressed by cockroachdb#82022. Informs cockroachdb#73743 Release note: None
This commit fixes a bug in `types.T.Identical` which caused it to return false for collated string types with a different string-representation of locales that represents the same locale logically. For example, collated string types with locales `en_US` and `en_us` would not be identical, even though these are both valid representations of the same locale. There is no release note because this has not caused any known bugs. Release note: None
47d6b6a
to
28a4887
Compare
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 should be ready for a look now.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @msirek, and @yuzefovich)
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Probably #73743.
Fixed, 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.
Reviewed 1 of 1 files at r1, 6 of 6 files at r3, 2 of 2 files at r4, 7 of 8 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @yuzefovich)
-- commits
line 34 at r5:
nit: "the correct the value" --> "the correct value"
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.
Is the fix for #73743 coming in a separate PR?
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @yuzefovich)
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.
Reviewed 1 of 1 files at r1, 6 of 6 files at r3, 2 of 2 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)
optbuilder wraps an assignment cast around a virtual computed column expression when writing to the table if the expression does not have the same type as the virtual column. This matches the behavior of all writes to columns of expressions that don't match the target column type. However, the same cast was not applied to the projection of the virtual column expression when reading from the table. This cast is necessary in order to produce the correct value of the column - the projection must produce the same value that would have been written to the table if the column was a stored computed column. This commit corrects optbuilder so that the cast is correctly added to the projection. Note that this commit adds a standard cast, not an assignment cast, as a temporary workaround until cockroachdb#81698 is addressed. This is because an assignment cast can fail in cases when a regular cast will not. Because we don't validate that all existing rows can successfully produce a new virtual column expression during a backfill, adding an assignment cast to the projection of the virtual column could cause future reads to error. Once cockroachdb#81698 is addressed, we can change these casts to assignment casts so that they directly match the values that would be written to the same column if it were stored. Fixes cockroachdb#91817 Informs cockroachdb#81698 Release note (bug fix): A bug has been fixed that could produce incorrect values for virtual computed columns in rare cases. The bug only presented when the virtual column expression's type did not match the type of the virtual column.
28a4887
to
437d4ef
Compare
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.
Is the fix for #73743 coming in a separate PR?
I have no plans to fix that one at the moment. I would like to address #81698 soon, though - it can cause lots of problems.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @yuzefovich)
Previously, msirek (Mark Sirek) wrote…
nit: "the correct the value" --> "the correct value"
Done.
TFTRs! bors r+ |
Build succeeded: |
blathers backport 23.1 |
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #91817: branch-release-23.1. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from ab5e955 to blathers/backport-release-23.1-105736: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
sql/logictest: remove assignment cast TODO
This commit removes a TODO that was partially addressed by #82022.
Informs #73743
Release note: None
sql/types: fix T.Identical
This commit fixes a bug in
types.T.Identical
which caused it to returnfalse for collated string types with a different string-representation
of locales that represents the same locale logically. For example,
collated string types with locales
en_US
anden_us
would not beidentical, even though these are both valid representations of the same
locale.
There is no release note because this has not caused any known bugs.
Release note: None
opt: wrap virtual computed column projections in a cast
optbuilder wraps an assignment cast around a virtual computed column
expression when writing to the table if the expression does not have the
same type as the virtual column. This matches the behavior of all writes
to columns of expressions that don't match the target column type.
However, the same cast was not applied to the projection of the virtual
column expression when reading from the table. This cast is necessary in
order to produce the correct value of the column - the projection must
produce the same value that would have been written to the table if the
column was a stored computed column. This commit corrects optbuilder so
that the cast is correctly added to the projection.
Note that this commit adds a standard cast, not an assignment cast, as a
temporary workaround until #81698 is addressed. This is because an
assignment cast can fail in cases when a regular cast will not. Because
we don't validate that all existing rows can successfully produce a new
virtual column expression during a backfill, adding an assignment cast
to the projection of the virtual column could cause future reads to
error. Once #81698 is addressed, we can change these casts to assignment
casts so that they directly match the values that would be written to
the same column if it were stored.
Fixes #91817
Informs #81698
Release note (bug fix): A bug has been fixed that could produce incorrect
values for virtual computed columns in rare cases. The bug only
presented when the virtual column expression's type did not match the
type of the virtual column.