opt: support UPDATE with partial UNIQUE WITHOUT INDEX constraints #60836
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)
pkg/sql/logictest/testdata/logic_test/unique, line 455 at r1 (raw file):
# Set a to a non-existing value. statement ok UPDATE uniq_partial SET a = 10 WHERE a = 9 AND b = 9
would be good to add one additional test that would fail if we had a full constraint, but succeeds with a partial constraint. For example, SET a = 1 WHERE b = -7
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file):
if _, isPartial := uc.Predicate(); isPartial { pred := mb.parseUniqueConstraintPredicateExpr(uniqueOrdinal) typedPred := mb.fetchScope.resolveAndRequireType(pred, types.Bool)
Are we now building this predicate multiple times? Is it worth caching it in the metadata like we do for indexes?
(Doesn't need to hold up this PR, but maybe open an issue if you think it's worth addressing at some point)
This commit add uniqueness checks for partial `UNIQUE WITHOUT INDEX` constraints during `UPDATE` statements. As partial of this change, I discovered that cockroachdb#60535 introduced a regression where columns not required by uniqueness checks are not pruned. I've left TODOs in the column pruning tests and plan on fixing this in a follow-up PR. There is no release note because these constraints are gated behind the experimental_enable_unique_without_index_constraints session variable. Release note: None
mgartner
force-pushed
the
partial-unique-update
branch
from
b5cc909
to
e5d499a
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/unique, line 455 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
would be good to add one additional test that would fail if we had a full constraint, but succeeds with a partial constraint. For example,
SET a = 1 WHERE b = -7
Done.
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Are we now building this predicate multiple times? Is it worth caching it in the metadata like we do for indexes?
(Doesn't need to hold up this PR, but maybe open an issue if you think it's worth addressing at some point)
Yes, there's a number of places where we build these expressions, like determining arbiters and uniqueness checks, and it's tricky to determine exactly how many times we are building the expression and what the minimum number could be. When add the expression to a relation expression in the memo, the scoping is critical. When we use the expression like we do here, to determine which column ordinals are referenced, the exact scoping isn't important, we just need some valid table scope to build the expression with.
In this case I actually tried to to use the predicate expression that is built with the WithScan scope or Scan scope of the uniqueness check, but it seemed to require a lot of refactoring given that this check happens before the uniqueness check is built.
I've created #60943 to track.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Yes, there's a number of places where we build these expressions, like determining arbiters and uniqueness checks, and it's tricky to determine exactly how many times we are building the expression and what the minimum number could be. When add the expression to a relation expression in the memo, the scoping is critical. When we use the expression like we do here, to determine which column ordinals are referenced, the exact scoping isn't important, we just need some valid table scope to build the expression with.
In this case I actually tried to to use the predicate expression that is built with the WithScan scope or Scan scope of the uniqueness check, but it seemed to require a lot of refactoring given that this check happens before the uniqueness check is built.
I've created #60943 to track.
Why can't we use TableMeta.PartialIndexPredicate()?
|
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file): Previously, RaduBerinde wrote…
In which case? In these particular lines, we can't use it because partial unique constraint predicates do not exist in |
|
TFTRs! bors r+ |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
In which case? In these particular lines, we can't use it because partial unique constraint predicates do not exist in
TableMeta.PartialIndexPredicate().
I see.. maybe we should add those to the meta as well? At a high level, I don't see why they would be treated differently
|
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 124 at r1 (raw file): Previously, RaduBerinde wrote…
That's definitely an option. The primary reason for adding partial index predicates to table metadata was so that the predicates could be accessed during exploration rules. No explorations rules need access to partial unique constraint predicates, so I never added them to the table metadata. These predicates are only needed in optbuilder. Also, by adding them to the table metadata, we may be able to use them in 1 or 2 places in optbuilder, but not in all places. When they are added as filters for uniqueness checks or for arbiter-related expressions, we must build them with a scope specific to the relational expression they are added to. So, adding them to table meta might simplify a few things, but it won't eliminate all the places where these expressions are built. I'll look more into it. |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
This commit add uniqueness checks for partial
UNIQUE WITHOUT INDEXconstraints during
UPDATEstatements.As partial of this change, I discovered that #60535 introduced a
regression where columns not required by uniqueness checks are not
pruned. I've left TODOs in the column pruning tests and plan on fixing
this in a follow-up PR.
There is no release note because these constraints are gated behind the
experimental_enable_unique_without_index_constraints session variable.
Release note: None