opt: build all partial index predicate expressions in TableMeta #57707
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @RaduBerinde, and @rytaft)
pkg/sql/opt/optbuilder/select.go, line 716 at r1 (raw file):
tableScope := b.allocScope() tableScope.appendOrdinaryColumnsFromTable(tabMeta, &tabMeta.Alias) tableScope.expr = b.factory.ConstructScan(&memo.ScanPrivate{
Ah.. constructing the extra Scan is unfortunate.. I guess we could get the FDs (memo.MakeTableFuncDep) etc and put them in a FakeRel but I don't know if that saves much.
We could pass an optional scan expression to the function, and have this code use it if its OutputCols contains tableScope.colSet(). Feels a bit hacky but it will save this construction in most cases.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, 1 of 1 files at r2.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/select.go, line 716 at r1 (raw file):
Previously, RaduBerinde wrote…
Ah.. constructing the extra Scan is unfortunate.. I guess we could get the FDs (
memo.MakeTableFuncDep) etc and put them in aFakeRelbut I don't know if that saves much.We could pass an optional scan expression to the function, and have this code use it if its OutputCols contains
tableScope.colSet(). Feels a bit hacky but it will save this construction in most cases.
In the common case wouldn't the ScanPrivates be the same? ThereforeConstructScan would return the memoized scan instead of building a new one?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/select.go, line 716 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
In the common case wouldn't the
ScanPrivates be the same? ThereforeConstructScanwould return the memoized scan instead of building a new one?
Nevermind... All the flags would have to be the same...
mgartner
force-pushed
the
always-build-partial-index-predicates
branch
from
892673d to
d320cf9
Compare
Previously, partial index predicates were only built and added to `TableMeta.PartialIndexPredicates` if the scope of the scan in `Builder.buildScan` included all ordinary table columns. This behavior prevented errors when building partial index predicate expressions for foriegn key check scans. These scans do not include all columns, so when building a predicate expression that referenced a table column not included, optbuilder would err with "column does not exist". This commit changes this behavior so that partial index predicates are always built and added to `TableMeta.PartialIndexPredicates`. In order to do this, `Builder.addPartialIndexPredicatesForTable` creates its own table scope. It also constructs a new scan if the provided scan does not output all column in the table scope. A scan on the table and its logical properties are required in order to fully normalize partial index predicates. See discussion at cockroachdb#57622 for more context on this change. Release note: None
Previously, the arbiter predicate provided in the `WHERE` clause of an `INSERT ... ON CONFLICT` statement was rebuilt as a `memo.FiltersExpr` repetitively while the optbuilder determined arbiter indexes. Now, the expression is built only once, reducing work. Release note: None
mgartner
force-pushed
the
always-build-partial-index-predicates
branch
from
d320cf9 to
9cc304b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/select.go, line 716 at r1 (raw file):
We could pass an optional scan expression to the function, and have this code use it if its OutputCols contains tableScope.colSet(). Feels a bit hacky but it will save this construction in most cases.
Done. Notice that the size in KB of the memos in the xform/select tests are no longer increased by this PR.
|
@RaduBerinde mind taking another look and make sure we're on the same page with this before I merge? |
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r+ |
|
Build succeeded: |
opt: build all partial index predicate expressions in TableMeta
Previously, partial index predicates were only built and added to
TableMeta.PartialIndexPredicatesif the scope of the scan inBuilder.buildScanincluded all ordinary table columns. This behaviorprevented errors when building partial index predicate expressions for
foriegn key check scans. These scans do not include all columns, so when
building a predicate expression that referenced a table column not
included, optbuilder would err with "column does not exist".
This commit changes this behavior so that partial index predicates are
always built and added to
TableMeta.PartialIndexPredicates. In orderto do this,
Builder.addPartialIndexPredicatesForTablecreates its owntable scope. It also constructs a new scan rather than relying on the
already-built scan. Constructing a scan is required for considering
logical properties while normalizing partial index predicates.
See discussion at #57622
for more context on this change.
Release note: None
optbuilder: reduce redundant building of arbiter filter expressions
Previously, the arbiter predicate provided in the
WHEREclause of anINSERT ... ON CONFLICTstatement was rebuilt as amemo.FiltersExprrepetitively while the optbuilder determined arbiter indexes. Now, the
expression is built only once, reducing work.
Release note: None