sql: fix identity computed columns with projection with generic plans#162036
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
ba9a4f6 to
dcb4e0e
Compare
20b3495 to
4eb513b
Compare
michae2
left a comment
There was a problem hiding this comment.
@michae2 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ZhouXing19).
Previously, when a computed column was defined as a identical to another column (e.g., vcol AS (col)), and a generic plan was used with a lookup join on an index containing that column (vcol), the query would fail with "cannot map variable %d to an indexed var". This occurred because the `Factory.Replace` function doesn't invoke the replacement callback for leaf nodes like `VariableExpr`, as it only traverses children. Thus, with the computed column expression a simple `VariableExpr`, table column references weren't getting replaced with their corresponding placeholder columns. As a result, when constructing the projection, we went through the MergeProjectWithValues rule incorrectly. This fix adds special handling in the constraint builder to directly apply the replacement function when the computed column expression is a simple VariableExpr, ensuring table columns are properly replaced with their corresponding placeholder columns. Fixes cockroachdb#161978 Release note (bug fix): Fixed an error that occurred when using generic plan that generates a lookup join on indexes containing identity computed columns.
4eb513b to
88d5777
Compare
|
Thanks for the prompt review! I also just realized i wrote the test under pkg/sql/opt/exec/execbuilder/testdata/virtual_columns, for i thought virtual column is a must to trigger this error. But turns out it is not, so i moved it to pkg/sql/opt/exec/execbuilder/testdata/prepare. |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice find!
@yuzefovich reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ZhouXing19).
|
TFTRs! |
|
bors r+ |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
bors r- |
|
GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like. |
|
bors r+ |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
cla check was stuck, so I nudged it bors r+ |
|
Thank you Yahor! ❤️ |
|
bors r+ |
|
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 #161978: branch-release-25.2, branch-release-25.3, branch-release-25.4, branch-release-26.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. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. error creating merge commit from 88d5777 to blathers/backport-release-25.2-162036: 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 25.2.x failed. See errors above. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. error creating merge commit from 88d5777 to blathers/backport-release-25.3-162036: 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 25.3.x failed. See errors above. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. error creating merge commit from 88d5777 to blathers/backport-release-25.4-162036: 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 25.4.x failed. See errors above. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. error creating merge commit from 88d5777 to blathers/backport-release-26.1-162036: 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 26.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Is this something we want to get into the next release train? The SHAs are picked on 02/04 and we'd need to backport into RC branches. |
Previously, when a computed column was defined as a identical to another column (e.g., vcol AS (col)), and a generic plan was used with a lookup join on an index containing that column (vcol), the query would fail with "cannot map variable %d to an indexed var".
This occurred because the original column is incorrectly included in the lookup join conditions.
For example, if we have
y_cp AS (y), with aWHERE x = $1::INT AND y = $2::INT, we are expecting($1 -> x, $2 -> y)as the lookup join conditions. However, the bug would wrongly include ay -> y_cp_eqprojection as well.
The root cause is that
Factory.Replacefunction doesn't invoke the replacement callback for leaf nodes likeVariableExpr, as it only traverses children. Thus, with the computed column expression a simpleVariableExpr, table column references weren't getting replaced with their corresponding placeholder columns. As a result, when constructing the projection, we went through theMergeProjectWithValuesrule incorrectly.This fix adds special handling in the constraint builder to directly apply the replacement function when the computed column expression is a simple VariableExpr, ensuring table columns are properly replaced with their corresponding placeholder columns.
Fixes #161978
Release note (bug fix): Fixed an error that occurred when using generic plan that generates a lookup join on indexes containing identity computed columns.