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: generate semi and anti inverted joins on multi-column indexes #58054

Merged

Conversation

mgartner
Copy link
Collaborator

This commit adds tests for generating semi and anti inverted joins on
multi-column inverted indexes. A ConstFilters field was added to
InvertedJoinPrivate, similar to LookupJoinPrivate so that row count
estimates for these expressions are more accurate. This was necessary to
make the semi and anti joins the chosen plans for the exploration tests.
A future commit will add more comprehensive stats tests.

Release note: None

@mgartner mgartner requested a review from rytaft December 18, 2020 02:36
@mgartner mgartner requested a review from a team as a code owner December 18, 2020 02:36
@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 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)


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


# Generate an inverted semi join on a multi-column inverted index.
opt expect=(GenerateInvertedJoinsFromSelect,ConvertSemiToInnerJoin)

[nit] I don't think ConvertSemiToInnerJoin is actually necessary here. We should probably get rid of that rule at some point (although I think we need to do some more work in GenerateLookupJoins first).

@mgartner mgartner force-pushed the semi-anti-inverted-join-multi-col branch from 1f5ef5e to 24bad8e Compare December 18, 2020 17:16
Copy link
Collaborator Author

@mgartner mgartner 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/testdata/rules/join, line 5349 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] I don't think ConvertSemiToInnerJoin is actually necessary here. We should probably get rid of that rule at some point (although I think we need to do some more work in GenerateLookupJoins first).

Done. I saw it here so I thought it might be important for inverted semi joins:

opt expect=(GenerateInvertedJoins,ConvertSemiToInnerJoin)

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 r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)


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

Previously, mgartner (Marcus Gartner) wrote…

Done. I saw it here so I thought it might be important for inverted semi joins:

opt expect=(GenerateInvertedJoins,ConvertSemiToInnerJoin)

Yea, that was probably left over from before Sumeer made the change to support semi joins directly with the paired joins.

This commit adds tests for generating semi and anti inverted joins on
multi-column inverted indexes. A `ConstFilters` field was added to
`InvertedJoinPrivate`, similar to `LookupJoinPrivate` so that row count
estimates for these expressions are more accurate. This was necessary to
make the semi and anti joins the chosen plans for the exploration tests.
A future commit will add more comprehensive stats tests.

Release note: None
@mgartner mgartner force-pushed the semi-anti-inverted-join-multi-col branch from 24bad8e to b047276 Compare December 18, 2020 22:01
Copy link
Collaborator Author

@mgartner mgartner 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/testdata/rules/join, line 5349 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Yea, that was probably left over from before Sumeer made the change to support semi joins directly with the paired joins.

Ok. I've removed it too!

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 1 of 1 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@mgartner
Copy link
Collaborator Author

TFTR!

bors r=rytaft

@craig craig bot merged commit e552218 into cockroachdb:master Dec 21, 2020
@craig
Copy link
Contributor

craig bot commented Dec 21, 2020

Build succeeded:

@mgartner mgartner deleted the semi-anti-inverted-join-multi-col branch December 21, 2020 21:50
mgartner added a commit to mgartner/cockroach that referenced this pull request Jan 20, 2021
The statistics builder was updated in cockroachdb#58054 to consider constant
filters on non-inverted prefix columns when building stats for
multi-column inverted joins. This commit adds additional tests for this.

There is no release note because multi-column inverted indexes are gated
behind the `experimental_enable_multi_column_inverted_indexes` session
setting.

Release note: None
craig bot pushed a commit that referenced this pull request Jan 20, 2021
58495: ui: changed metric label CPUs to vCPUs r=nanduu04 a=nanduu04

Release note (ui change): CPUs metric column renamed to vCPUs in node
overview in cluster overview page.

59171: opt: build stats and logical props for multi-column inverted joins r=rytaft a=mgartner

#### opt: add stats tests for multi-column inverted joins

The statistics builder was updated in #58054 to consider constant
filters on non-inverted prefix columns when building stats for
multi-column inverted joins. This commit adds additional tests for this.

There is no release note because multi-column inverted indexes are gated
behind the `experimental_enable_multi_column_inverted_indexes` session
setting.

Release note: None

#### opt: consider inverted join prefix key columns when building logical props

This commit updates the logical props builder to consider inverted join
prefix key columns when building logical props for inverted join
expressions.

There is no release note because multi-column inverted indexes are gated
behind the `experimental_enable_multi_column_inverted_indexes` session
setting.

Release note: None


Co-authored-by: Nandu Pokhrel <nandup@cockroachlabs.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
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