opt: do not provide project ordering for columns removed when simplified #64444
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
There was a problem hiding this comment.
Reviewable status:
|
pkg/sql/opt/ordering/project_test.go, line 81 at r1 (raw file):
Is it worth also adding a data-driven regression test with a SQL query affected by this problem? Or is there no good reproduction? |
Previously, `projectCanProvideOrdering` did not simplify an ordering base on an FD set if the original ordering could be provided, while `projectBuildChildReqOrdering` always attempted to simplify an ordering. In cockroachdb#64342 the behavior of `OrderingChoice.Simplify` changed to remove columns in groups that are not known to be equivalent by the FD. As a result, `projectCanProvideOrdering` could return true in cases where the simplified ordering, with some columns removed, could not be provided, causing `ordering.projectBuildChildReqOrdering` to panic. This commit updates `ordering.projectCanProvideOrdering` so that the ordering is always simplified, matching the behavior of `ordering.projectBuildChildReqOrdering`. Fixes cockroachdb#64399 Release note: None
mgartner
force-pushed
the
64399-fix-project-ordering
branch
from
2c74904 to
6d36413
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/ordering/project_test.go, line 81 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Is it worth also adding a data-driven regression test with a SQL query affected by this problem? Or is there no good reproduction?
I left it out because its pretty non-sensical and it didn't really belong anywhere. But I found logictest/testdata/logic_test/sqlsmith which seems like a loving home for it. 🏡
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:
pkg/sql/opt/ordering/project_test.go, line 81 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I left it out because its pretty non-sensical and it didn't really belong anywhere. But I found
logictest/testdata/logic_test/sqlsmithwhich seems like a loving home for it. 🏡
Hehe awesome
|
TFTRs! bors r+ |
|
Build succeeded: |
Previously,
projectCanProvideOrderingdid not simplify an orderingbase on an FD set if the original ordering could be provided, while
projectBuildChildReqOrderingalways attempted to simplify an ordering.In #64342 the behavior of
OrderingChoice.Simplifychanged to removecolumns in groups that are not known to be equivalent by the FD. As a
result,
projectCanProvideOrderingcould return true in cases where thesimplified ordering, with some columns removed, could not be provided,
causing
ordering.projectBuildChildReqOrderingto panic.This commit updates
ordering.projectCanProvideOrderingso that theordering is always simplified, matching the behavior of
ordering.projectBuildChildReqOrdering.Fixes #64399
Release note: None