opt: remove TableMeta.IsSkipLocked#85933
Conversation
|
This change is |
rytaft
left a comment
There was a problem hiding this comment.
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)
michae2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 5 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
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. 🤔
msirek
left a comment
There was a problem hiding this comment.
Reviewable status:
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?
mgartner
left a comment
There was a problem hiding this comment.
Reviewable status:
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.
michae2
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
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
verifyFiltersAreValidEqualitiesjust 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. |
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
michae2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:
|
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
TestGetJoinMultiplicitywas a duplicate of test case 2.It has been updated to be similar, but use an
INNER JOINinstead 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.IsSkipLockedwas used by the multiplicity builder todetermine whether a column was filtered or not by a
SKIP LOCKEDscan.However, this field was unnecessary. The
deriveUnfilteredColsfunctionin the multiplicity builder already traverses expressions all the way
down to
ScanExprs, only collecting unfiltered columns if theyoriginate from a scan where
ScanExpr.IsUnfilteredreturns true.ScanExpr.IsUnfilteredreturns false if the scan is aSKIP LOCKEDscan. Therefore, no additional mechanism is required to detect columns
filtered by
SKIP LOCKEDscans.I noticed that
TableMeta.IsSkipLockedwas 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 LOCKEDis 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