Skip to content

opt: plan lookup anti-, semi-, and leftjoins on indexes with virtual columns#76697

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:anti-semi-lookup-join
Feb 17, 2022
Merged

opt: plan lookup anti-, semi-, and leftjoins on indexes with virtual columns#76697
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:anti-semi-lookup-join

Conversation

@mgartner
Copy link
Copy Markdown
Contributor

@mgartner mgartner commented Feb 16, 2022

Fixes #75873

Release note: None

@mgartner mgartner requested a review from a team as a code owner February 16, 2022 18:23
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Feb 16, 2022

This change is Reviewable

@mgartner mgartner requested a review from rytaft February 16, 2022 18:24
chengxiong-ruan
chengxiong-ruan approved these changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

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

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 666 at r1 (raw file):

                          spans: /3/8765/0

# TODO(mgartner): The lookup join that checks for conflicts performs a lookup

Thanks for adding this TODO! it also applies to several other tests in this file and new tests from my pr #76358 :)
Do you plan to fix it for 22.1?

rytaft
rytaft approved these changes Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: good find!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @RaduBerinde)

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 16, 2022

pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index, line 666 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Thanks for adding this TODO! it also applies to several other tests in this file and new tests from my pr #76358 :)
Do you plan to fix it for 22.1?

Yes, I plan on fixing it.

Unfortunately, I think the solution is to undo your previous changes which added the UNIQUE WITHOUT INDEX constraint to the table catalog. This is what's causing the issue, because it means that we select the UNIQUE WITHOUT INDEX as the arbiter, and therefore we build a left join with a filter that only has equality filters for the non-shard columns. Instead, we want to select the unique index with the shard columns as the arbiter, so that an equality condition on the shard column is included in the left join.

I'm sure I'll run into some problems that were handled easily by the synthetic UNIQUE WITHOUT INDEX constraint, but I don't see another way around removing it.

@mgartner mgartner force-pushed the anti-semi-lookup-join branch from 7e5cf74 to ffcf8ac Compare February 16, 2022 20:58
msirek
msirek approved these changes Feb 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan and @RaduBerinde)

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 17, 2022

TFTRs!

bots r+

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Feb 17, 2022

"bots" 🤦‍♂️

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 17, 2022

Build succeeded:

@craig craig bot merged commit f2a722f into cockroachdb:master Feb 17, 2022
@mgartner mgartner deleted the anti-semi-lookup-join branch February 17, 2022 21:03
@yuzefovich yuzefovich mentioned this pull request Nov 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rytaft rytaft rytaft approved these changes

@RaduBerinde RaduBerinde Awaiting requested review from RaduBerinde

+2 more reviewers

@msirek msirek msirek approved these changes

@chengxiong-ruan chengxiong-ruan chengxiong-ruan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

opt: plan lookup anti-, semi-, and left joins on indexes with virtual columns

5 participants

@mgartner @cockroach-teamcity @rytaft @msirek @chengxiong-ruan