opt: remove non-equivalent group columns in OrderingChoice.Simplify #64342
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
RaduBerinde
approved these changes
Apr 28, 2021
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)
pkg/sql/opt/props/physical/ordering_choice.go, line 523 at r1 (raw file):
} // Check whether new equivalent columns can be added by the FD set.
[nit] Check whether the equivalency group needs to change.
Previously, `OrderingChoice.Simplify` would add but never remove columns from ordering column groups based on equivalency in an FD. In rare cases, this could cause the optimizer to generate expressions which violated an invariant that all columns in an ordering column group are equivalent according to the expression's FD. Violation of this invariant only panics in test builds, and in the test cases found that trigger this panic, there is likely no correctness issues with the expression. Therefore, there was probably no impact in any release builds. This commit updates `OrderingChoice.Simplify` so that non-equivalent columns in an ordering column group are removed from the group, satisfying the invariant. Fixes cockroachdb#63794 Release note: None
mgartner
force-pushed
the
63794-ordering-fix
branch
from
a0dd8b4 to
ecfff8c
Compare
mgartner
commented
Apr 28, 2021
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/props/physical/ordering_choice.go, line 523 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] Check whether the equivalency group needs to change.
Done.
rytaft
approved these changes
Apr 28, 2021
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Apr 30, 2021
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
added a commit
to mgartner/cockroach
that referenced
this pull request
Apr 30, 2021
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
craig bot
pushed a commit
that referenced
this pull request
Apr 30, 2021
64306: pgcode: remove a couple of deprecated errors r=yuzefovich a=yuzefovich Release note: None 64441: ccl/sqlproxyccl: fix a broken test in bazel r=darinpp a=darinpp This fixes #61915 by adding the required data files. Release note: None 64444: opt: do not provide project ordering for columns removed when simplified r=mgartner a=mgartner 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 #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 #64399 Release note: None 64483: bazel: prefer `exec_tools` to `tools` in `BUILD` files r=rail a=rickystewart The [documentation](https://docs.bazel.build/versions/master/be/general.html#genrule_args) specifies that `exec_tools` is preferred to `tools` wherever possible. It doesn't matter in our case, so let's just standardize on `exec_tools` everywhere. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Darin Peshev <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
May 13, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously,
OrderingChoice.Simplifywould add but never remove columnsfrom ordering column groups based on equivalency in an FD. In rare
cases, this could cause the optimizer to generate expressions which
violated an invariant that all columns in an ordering column group are
equivalent according to the expression's FD.
Violation of this invariant only panics in test builds, and in the test
cases found that trigger this panic, there is likely no correctness
issues with the expression. Therefore, there was probably no impact in
any release builds.
This commit updates
OrderingChoice.Simplifyso that non-equivalentcolumns in an ordering column group are removed from the group,
satisfying the invariant.
Fixes #63794
Release note: None