-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: remove TableMeta.IsSkipLocked #85933
Conversation
There was a problem hiding this 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 r1, 5 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @msirek)
There was a problem hiding this 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 r1, 5 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @msirek)
pkg/sql/opt/memo/multiplicity_builder.go
line 280 at r2 (raw file):
} if checkSelfJoinCase(left.Memo().Metadata(), filters) { // Case 2a.
Is this self-join case handled? It looks like the other cases are, thanks to the cardinality checks and the use of unfiltered cols, but I don't see how this self-join case is handled.
But the cases you added to multiplicity_builder_test seem to cover this, so it must be handled somehow. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 2 of 0 LGTMs obtained (waiting on @mgartner)
pkg/sql/opt/memo/logical_props_builder.go
line 523 at r3 (raw file):
// this provides extra safety. rel.Cardinality = rel.Cardinality.AsLowAs(0) }
Should similar code be added to buildIndexJoinProps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @michae2, @msirek, and @rytaft)
pkg/sql/opt/memo/logical_props_builder.go
line 523 at r3 (raw file):
Previously, msirek (Mark Sirek) wrote…
Should similar code be added to
buildIndexJoinProps
?
Yes! It was already added in #85720.
pkg/sql/opt/memo/multiplicity_builder.go
line 280 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Is this self-join case handled? It looks like the other cases are, thanks to the cardinality checks and the use of unfiltered cols, but I don't see how this self-join case is handled.
But the cases you added to multiplicity_builder_test seem to cover this, so it must be handled somehow. 🤔
This confused me too. IIRC it is handled by verifyFiltersAreValidEqualities
just above here. If the columns in the equality are filtered on the right input, then we never actually make it here - we return false 3 lines up.
We check for valid equality filters before we check the self join (2a) or FK (2b) case, because both cases have the same requirements about the filters. This paragraph in the comment calls this out:
// In both the self-join and the foreign key cases, the left columns must be
// not-null, and the right columns must be either unfiltered, or the left and
// right must be Select expressions where the left side filters imply the right
// side filters and right columns are unfiltered in the right Select's input
// (see condition #3b in the comment for verifyFiltersAreValidEqualities).
I found this hard to follow (and I wrote it!), so I added a new commit that rewrites this in an attempt to be more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner, @michae2, @msirek, and @rytaft)
pkg/sql/opt/memo/multiplicity_builder.go
line 280 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
This confused me too. IIRC it is handled by
verifyFiltersAreValidEqualities
just above here. If the columns in the equality are filtered on the right input, then we never actually make it here - we return false 3 lines up.We check for valid equality filters before we check the self join (2a) or FK (2b) case, because both cases have the same requirements about the filters. This paragraph in the comment calls this out:
// In both the self-join and the foreign key cases, the left columns must be // not-null, and the right columns must be either unfiltered, or the left and // right must be Select expressions where the left side filters imply the right // side filters and right columns are unfiltered in the right Select's input // (see condition #3b in the comment for verifyFiltersAreValidEqualities).
I found this hard to follow (and I wrote it!), so I added a new commit that rewrites this in an attempt to be more clear.
I see, that makes sense. Thank you!
Ah, thanks.. I didn't see it. I was reviewing code in a branch that's a few days old. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner, @michae2, @msirek, and @rytaft)
8b6c1e2
to
a93cd43
Compare
Test case 9 in `TestGetJoinMultiplicity` was a duplicate of test case 2. It has been updated to be similar, but use an `INNER JOIN` instead of a `LEFT JOIN`, which I believe it was originally supposed to be - there is no similar existing test case. Release note: None
`TableMeta.IsSkipLocked` was used by the multiplicity builder to determine whether a column was filtered or not by a `SKIP LOCKED` scan. However, this field was unnecessary. The `deriveUnfilteredCols` function in the multiplicity builder already traverses expressions all the way down to `ScanExpr`s, only collecting unfiltered columns if they originate from a scan where `ScanExpr.IsUnfiltered` returns true. `ScanExpr.IsUnfiltered` returns false if the scan is a `SKIP LOCKED` scan. Therefore, no additional mechanism is required to detect columns filtered by `SKIP LOCKED` scans. I noticed that `TableMeta.IsSkipLocked` was not needed because the multiplicity builder unit tests added in cockroachdb#85720 never set it, yet the tests passed. This commit also adds more multiplicity builder unit tests for self-joins with `SKIP LOCKED`. Release note: None
This commit ensures that the minimum cardinality of a scan or lookup-join with `SKIP LOCKED` is zero. This shouldn't be needed because the logical to derive the cardinality for these expressions should never produce a non-zero minimum cardinality, but this provides extra safety. Release note: None
Release note: None
There was a problem hiding this 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 r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner and @msirek)
bors r+ |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed: |
bors r+ |
Build succeeded: |
opt: fix duplicate join multiplicity test
Test case 9 in
TestGetJoinMultiplicity
was a duplicate of test case 2.It has been updated to be similar, but use an
INNER JOIN
instead of aLEFT JOIN
, which I believe it was originally supposed to be - there isno similar existing test case.
Release note: None
opt: remove TableMeta.IsSkipLocked
TableMeta.IsSkipLocked
was used by the multiplicity builder todetermine whether a column was filtered or not by a
SKIP LOCKED
scan.However, this field was unnecessary. The
deriveUnfilteredCols
functionin the multiplicity builder already traverses expressions all the way
down to
ScanExpr
s, only collecting unfiltered columns if theyoriginate from a scan where
ScanExpr.IsUnfiltered
returns true.ScanExpr.IsUnfiltered
returns false if the scan is aSKIP LOCKED
scan. Therefore, no additional mechanism is required to detect columns
filtered by
SKIP LOCKED
scans.I noticed that
TableMeta.IsSkipLocked
was not needed because themultiplicity builder unit tests added in #85720 never set it, yet the
tests passed.
This commit also adds more multiplicity builder unit tests for
self-joins with
SKIP LOCKED
.Release note: None
opt: ensure min cardinality of zero for expressions with SKIP LOCKED
This commit ensures that the minimum cardinality of a scan or
lookup-join with
SKIP LOCKED
is zero. This shouldn't be needed becausethe logical to derive the cardinality for these expressions should never
produce a non-zero minimum cardinality, but this provides extra safety.
Release note: None
opt: improve comments in multiplicity builder
Release note: None