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: use paired joins for left semi inverted joins #55986

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

sumeerbhola
Copy link
Collaborator

@sumeerbhola sumeerbhola commented Oct 26, 2020

Semi joins using the inverted index used to be converted
to an inner inverted join followed by an inner lookup join
and a distinct for de-duplication. They are now converted
to paired-joins consisting of an inner inverted join
followed by a left semi lookup join, which is more
efficient.

Release note: None

@sumeerbhola sumeerbhola requested a review from a team as a code owner October 26, 2020 18:07
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
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:

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


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

		continuationCol := opt.ColumnID(0)
		invertedJoinType := joinType
		// Anti/semi joins are converted to a pair consisting of a left join and

is there any way we can use an inner join for the first join when the second is a semi join?


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

# custom function for more details.
# TODO(rytaft): Add support for SemiJoin. Currently it is supported by first
# converting it to InnerJoin using the rule ConvertSemiToInnerJoin.

remove TODO

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

TFTR!

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


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

Previously, rytaft (Rebecca Taft) wrote…

is there any way we can use an inner join for the first join when the second is a semi join?

That was silly of me. I thought I'd forgotten to add support for a continuation column for inner joins, but I had actually added that for exactly this reason (and it's even explained in the comment for InvertedJoinerSpec).

Done.


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

Previously, rytaft (Rebecca Taft) wrote…

remove TODO

Done

Copy link
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.

Nice! :lgtm:

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@rytaft
Copy link
Collaborator

rytaft commented Oct 26, 2020


pkg/sql/opt/xform/join_funcs.go, line 475 at r2 (raw file):

			// Semi joins are converted to a pair consisting of an inner inverted
			// join and semi lookup join.
			continuationCol = c.constructContinuationColumnForPairedLeftJoin()

[nit] maybe rename this function to just be PairedJoin instead of PairedLeftJoin

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/opt/xform/join_funcs.go, line 475 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] maybe rename this function to just be PairedJoin instead of PairedLeftJoin

The original intention was for "left join" to refer to the overall join (which is a left semi/anti/outer join). But I see why this can be confusing. I've renamed it as you suggested, and added a sentence to the function comment.

Copy link
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.

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

Semi joins using the inverted index used to be converted
to an inner inverted join followed by an inner lookup join
and a distinct for de-duplication. They are now converted
to paired-joins consisting of an inner inverted join
followed by a left semi lookup join, which is more
efficient.

Release note: None
@rytaft
Copy link
Collaborator

rytaft commented Oct 27, 2020

Don't forget to update the PR message to change left outer -> inner

Copy link
Collaborator Author

@sumeerbhola sumeerbhola 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 the reminder. Done. I wish it would sync from the commit message for a single commit PR.

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

@rytaft
Copy link
Collaborator

rytaft commented Oct 27, 2020

Yea... that would be nice

@sumeerbhola
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 27, 2020

Build succeeded:

@craig craig bot merged commit 8652b7a into cockroachdb:master Oct 27, 2020
@sumeerbhola sumeerbhola deleted the paired_semi branch November 2, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants