opt: fix constant columns bug in zig-zag join rule #48128
Merged
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 |
mgartner
approved these changes
Apr 28, 2020
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @rytaft)
a discussion (no related file):
andy-kimball
approved these changes
Apr 28, 2020
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, mgartner (Marcus Gartner) wrote…
:lgtm:
pkg/sql/opt/constraint/constraint_set.go, line 294 at r1 (raw file):
// ExtractValueForConstColumn extracts the value for a constant column returned // by ExtractConstCols. func (s *Set) ExtractValueForConstColumn(evalCtx *tree.EvalContext, col opt.ColumnID) tree.Datum {
NIT: would call this ExtractValueForConstCol for symmetry.
This is a fix for a regression introduced by cockroachdb#47412. We were using `ExtractConstCols` to get the list of possible fixed columns, and later we were using `findConstantFilter` to get the values. The latter has more restrictive semantics so we wouldn't always find the value we need. The fix is to use logic consistent with `ExtractConstCols`, which is now added as `ExtractValueForConstColumn`. The zig-zag join code now also verifies that we fill in all the columns so we get a better error earlier. Fixes cockroachdb#48003. Release note (bug fix): Fixed "non-values node passed as fixed value to zigzag join" internal error.
RaduBerinde
force-pushed
the
fix-zig-zag-bug
branch
from
0f0f18b
to
2210d93
Compare
RaduBerinde
commented
Apr 29, 2020
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
Build succeeded |
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.
This is a fix for a regression introduced by #47412.
We were using
ExtractConstColsto get the list of possible fixedcolumns, and later we were using
findConstantFilterto get the values.The latter has more restrictive semantics so we wouldn't always find
the value we need.
The fix is to use logic consistent with
ExtractConstCols, which isnow added as
ExtractValueForConstColumn. The zig-zag join code nowalso verifies that we fill in all the columns so we get a better error
earlier.
Fixes #48003.
Release note (bug fix): Fixed "non-values node passed as fixed value
to zigzag join" internal error.