Skip to content
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

opt: only plan zigzag joins against lax key cols #36139

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
4 participants
@justinj
Copy link
Member

commented Mar 25, 2019

Fixes #36051.

The zigzag joiner today only supports zigzagging across a prefix of the
key columns of an index. Previously, we would check the entire column
set of the index, which would cause problems when we tried to zigzag on
columns which weren't guaranteed to be present in the key part of the
index.

Release note (bug fix): fixed a panic when planning zigzag joins against
unique indexes.

@justinj justinj requested review from rytaft and RaduBerinde Mar 25, 2019

@justinj justinj requested a review from cockroachdb/sql-opt-prs as a code owner Mar 25, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

This change is Reviewable

@RaduBerinde
Copy link
Member

left a comment

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)


pkg/sql/opt/xform/testdata/rules/join, line 1707 at r1 (raw file):

      └── s = 'foo' [type=bool, outer=(4), constraints=(/4: [/'foo' - /'foo']; tight), fd=()-->(4)]

# Don't generate a zigzag against nullable unique indexes whose primary key

"whose primary key might not be present" is very confusing. Though it seems pretty hard to explain.. maybe "unique indexes where the PK is not part of the indexed columns"

@rytaft

rytaft approved these changes Mar 26, 2019

Copy link
Contributor

left a comment

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @justinj)


pkg/sql/opt/xform/custom_funcs.go, line 853 at r1 (raw file):

	rightEqPrefix = make(opt.ColList, 0, len(rightEqCols))
	// Use the LaxKeyColumnCount here because that's the longest prefix of the
	// columns in the index which is guaranteed to exist in the key component.

I'm a bit confused about this... can you give an example of what could go wrong?


pkg/sql/opt/xform/testdata/rules/join, line 127 at r1 (raw file):

    b INT8 NULL,
    c INT8 NULL,
    INDEX idx_a (b ASC),

[nit] maybe call this idx_b?


pkg/sql/opt/xform/testdata/rules/join, line 1709 at r1 (raw file):

# Don't generate a zigzag against nullable unique indexes whose primary key
# might not be present.
# Regression test for #36051: prior, we would try to use the PK as the

[nit] prior -> prior to fixing this issue

opt: only plan zigzag joins against lax key cols
Fixes #36051.

The zigzag joiner today only supports zigzagging across a prefix of the
key columns of an index. Previously, we would check the entire column
set of the index, which would cause problems when we tried to zigzag on
columns which weren't guaranteed to be present in the key part of the
index.

Release note (bug fix): fixed a panic when planning zigzag joins against
unique indexes.

@justinj justinj force-pushed the justinj:fix-zz-2 branch from 34993fd to 63d55b1 Mar 27, 2019

@justinj

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Mar 27, 2019

Merge #36139
36139: opt: only plan zigzag joins against lax key cols r=justinj a=justinj

Fixes #36051.

The zigzag joiner today only supports zigzagging across a prefix of the
key columns of an index. Previously, we would check the entire column
set of the index, which would cause problems when we tried to zigzag on
columns which weren't guaranteed to be present in the key part of the
index.

Release note (bug fix): fixed a panic when planning zigzag joins against
unique indexes.

Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

commented Mar 27, 2019

Build succeeded

@craig craig bot merged commit 63d55b1 into cockroachdb:master Mar 27, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.