opt/idxconstraint: recognize stored column expressions#55867
opt/idxconstraint: recognize stored column expressions#55867craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
e60ff91 to
5f77421
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 6 of 6 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)
pkg/sql/opt/idxconstraint/index_constraints.go, line 1428 at r2 (raw file):
} // isIndexColumn returns true if ev is a variable on the n indexed var that
I think this comment is a bit stale
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 6 of 6 files at r2.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/select, line 1646 at r2 (raw file):
└── scan computed@l_idx ├── columns: k:1!null ├── constraint: /3/1: [/'foo' - /'foo']
Maybe the formatted constraint should include the column name, like /l:3/k:1? The column ID 3 appears no where else in this output, so it's not clear that it is l. No need to make this change for this PR, but something we should think about.
Release note: None
66ab240 to
1f6de5a
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/idxconstraint/index_constraints.go, line 1428 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I think this comment is a bit stale
Done.
pkg/sql/opt/xform/testdata/rules/select, line 1646 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Maybe the formatted constraint should include the column name, like
/l:3/k:1? The column ID3appears no where else in this output, so it's not clear that it isl. No need to make this change for this PR, but something we should think about.
Good idea, I'll look at that separately.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 9 files at r3, 6 of 6 files at r4.
Reviewable status:
pkg/sql/opt/idxconstraint/index_constraints.go, line 1428 at r2 (raw file):
Previously, RaduBerinde wrote…
Done.
It still references ev which doesn't exist.
This change makes the index constraint generator aware of computed column expressions so it can substitute them for the computed column if it is indexed. Release note (performance improvement): indexes on computed columns can now be utilized when filters reference the computed expression and not the computed column directly.
1f6de5a to
cad8242
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/idxconstraint/index_constraints.go, line 1428 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
It still references
evwhich doesn't exist.
Fixed.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status:
|
bors r+ |
1 similar comment
|
bors r+ |
|
Already running a review |
|
Build succeeded: |
deps: update datadriven
Release note: None
opt/idxconstraint: recognize stored column expressions
This change makes the index constraint generator aware of computed
column expressions so it can substitute them for the computed column
if it is indexed.
Release note (performance improvement): indexes on computed columns
can now be utilized when filters reference the computed expression and
not the computed column directly.