-
Notifications
You must be signed in to change notification settings - Fork 4.1k
opt: fix PruneUnionAllCols panic with outer scope columns #160580
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
Conversation
Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes cockroachdb#159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com>
michae2
left a 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.
@michae2 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball).
|
TFTR! bors r=michae2 |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
|
Build failed: |
|
courtesy merge bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com> 160632: sql/bulkmerge: reuse SST iterator across bulk merge tasks r=spilchen a=spilchen This change reduces overhead in the bulk merge processor by initializing a single iterator over all input SSTs at startup, rather than creating a new one per task. The iterator is reused across tasks, seeking only when needed. Informs #159414 Epic: CRDB-48845 Release note: none Co-authored by: `@jeffswenson` Co-authored-by: Drew Kimball <drewk@cockroachlabs.com> Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
|
Build failed (retrying...): |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
|
Build failed: |
|
courtesy merge 2 bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
|
Build failed:
|
|
courtesy merge 3 bors retry |
160580: opt: fix PruneUnionAllCols panic with outer scope columns r=michae2 a=DrewKimball Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns. This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic. Fixes #159793 Release note: None Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
|
Build failed: |
|
courtesy merge 4 bors retry |
Before this change, the PruneUnionAllCols normalization rule would panic in crdb-test builds when the projection above a UnionAll referenced columns from an outer scope (e.g., due to an apply-join or routine). This occurred because the rule computed the needed column set by combining ProjectionOuterCols and passthrough columns, which could include outer scope columns not present in the UnionAll's output. These outer columns were then passed to NeededColMapLeft/Right, which call TranslateColSetStrict and panic when given unknown columns.
This change fixes the issue by intersecting the needed column set with the UnionAll's output columns before passing it to NeededColMapLeft/ Right. This ensures only columns actually present in the UnionAll are translated, preventing the panic.
Fixes #159793
Release note: None
Co-Authored-By: Claude noreply@anthropic.com