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: add ScanPrivate.IsUnfiltered() helper function #50784

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

mgartner
Copy link
Collaborator

This commit adds a new method to ScanPrivate, IsUnfiltered, which
returns true if the scan is guaranteed to produce all rows in the table.
It returns false if it may not return all rows because it has a limit,
it is constrained, or it scans a partial index.

This commit updates a few lines of code that were manually performing
these checks.

Related context: #49933 (comment)

Release note: None

@mgartner mgartner requested a review from andy-kimball June 29, 2020 20:49
@mgartner mgartner requested a review from a team as a code owner June 29, 2020 20:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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 (waiting on @andy-kimball)


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

	// estimate turns out to be smaller than the actual row count.
	var preferConstrainedScanCost memo.Cost
	if scan.IsUnfiltered(c.mem.Metadata()) {

By using IsUnfiltered here, a Scan with a limit is also exempt from receiving this slight cost increase. I don't think this is a big issue, because if the cost was equal for two plans, one with a Limit and one without, there shouldn't be any issue with preferring the limited Scan. But LMK if I'm wrong about this.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @mgartner)


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

Previously, mgartner (Marcus Gartner) wrote…

By using IsUnfiltered here, a Scan with a limit is also exempt from receiving this slight cost increase. I don't think this is a big issue, because if the cost was equal for two plans, one with a Limit and one without, there shouldn't be any issue with preferring the limited Scan. But LMK if I'm wrong about this.

Yeah this should be fine.


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

	scanPrivate *memo.ScanPrivate, required physical.OrderingChoice,
) bool {
	// Don't push a limit into the scan if the scan is already filtered by a

The comment is the opposite of what we're doing..

I think the old code is what we want here.. we'll want to allow pushing a limit into a constrained partial index scan.

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! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)


pkg/sql/opt/xform/custom_funcs.go, line 1202 at r1 (raw file):
Hmm looks like I got a bit confused here.

I think the old code is what we want here.. we'll want to allow pushing a limit into a constrained partial index scan.

But we'll also want to allow pushing a limit into an unconstrained partial index scan, correct? In that case I think it'd look something like:

if scanPrivate.HardLimit != 0 {
    return false
}

if scanPrivate.IsUnfiltered {
    return false
}

Or maybe we should be pushing a limit into any non-limited, non-canonical scan? The comments mention PushLimitIntoScan (no longer a rule) pushing a limit into unconstrained scans. But GenerateLimitedScans only operates on canonical scans, not all unconstrained scans.

Copy link
Member

@RaduBerinde RaduBerinde 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! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @mgartner)


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

// satisfied by the Scan operator.
//
// NOTE: Limiting canonical scans is done by the GenerateLimitedScans rule,

I think this should still say "unconstrained scans"


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

Previously, mgartner (Marcus Gartner) wrote…

Hmm looks like I got a bit confused here.

I think the old code is what we want here.. we'll want to allow pushing a limit into a constrained partial index scan.

But we'll also want to allow pushing a limit into an unconstrained partial index scan, correct? In that case I think it'd look something like:

if scanPrivate.HardLimit != 0 {
    return false
}

if scanPrivate.IsUnfiltered {
    return false
}

Or maybe we should be pushing a limit into any non-limited, non-canonical scan? The comments mention PushLimitIntoScan (no longer a rule) pushing a limit into unconstrained scans. But GenerateLimitedScans only operates on canonical scans, not all unconstrained scans.

This rule is specific to constrained scans. GenerateLimitedScans considers all indexes so it handles the unconstrained case.

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 @andy-kimball, @mgartner, and @RaduBerinde)


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

Previously, RaduBerinde wrote…

This rule is specific to constrained scans. GenerateLimitedScans considers all indexes so it handles the unconstrained case.

Ahh, don't know how I missed that. However, GenerateLimitedScans cannot handle unconstrained partial index scans because it doesn't deal with any filters, and unconstrained partial index scans can only be generated when there is a filter.

I'll leave this unchanged in this PR for now. But I think there are two options for generating limited, unconstrained, partial index scans:

  1. Add a new rule GenerateLimitedPartialIndexScans that matches a (Limit (Select (Scan))).

  2. Modify PushLimitIntoConstrainedSan to handle unconstrained partial index scans.

Copy link
Member

@RaduBerinde RaduBerinde 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 @andy-kimball and @mgartner)


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

Previously, mgartner (Marcus Gartner) wrote…

Ahh, don't know how I missed that. However, GenerateLimitedScans cannot handle unconstrained partial index scans because it doesn't deal with any filters, and unconstrained partial index scans can only be generated when there is a filter.

I'll leave this unchanged in this PR for now. But I think there are two options for generating limited, unconstrained, partial index scans:

  1. Add a new rule GenerateLimitedPartialIndexScans that matches a (Limit (Select (Scan))).

  2. Modify PushLimitIntoConstrainedSan to handle unconstrained partial index scans.

I see. The reason why things are like this is that without GenerateLimitedScans, we may not create expressions for the relevant indexes in the first place (at least in the case where they require an index join). This is not a problem for partial index scans - they are created because we have a filter that matches the predicate. So I believe (2) would be the right thing.

@mgartner mgartner force-pushed the is-unfiltered branch 2 times, most recently from 8c06ea0 to 05c66fc Compare June 30, 2020 20:27
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 @andy-kimball)


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

Previously, RaduBerinde wrote…

I think this should still say "unconstrained scans"

Done.


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

Previously, RaduBerinde wrote…

I see. The reason why things are like this is that without GenerateLimitedScans, we may not create expressions for the relevant indexes in the first place (at least in the case where they require an index join). This is not a problem for partial index scans - they are created because we have a filter that matches the predicate. So I believe (2) would be the right thing.

Sounds good to me. I created an issue to track: #50828

This commit adds a new method to `ScanPrivate`, `IsUnfiltered`, which
returns true if the scan is guaranteed to produce all rows in the table.
It returns false if it may not return all rows because it has a limit,
it is constrained, or it scans a partial index.

This commit updates a few lines of code that were manually performing
these checks.

Release note: None
@mgartner
Copy link
Collaborator Author

mgartner commented Jul 1, 2020

bors r=RaduBerinde

@craig
Copy link
Contributor

craig bot commented Jul 1, 2020

Build succeeded

@craig craig bot merged commit 54c7f57 into cockroachdb:master Jul 1, 2020
@mgartner mgartner deleted the is-unfiltered branch July 1, 2020 21:06
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