opt: constrain multi-col inverted index prefix cols by contiguous range#56835
opt: constrain multi-col inverted index prefix cols by contiguous range#56835craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:
pkg/sql/opt/invertedidx/inverted_index_expr.go, line 303 at r1 (raw file):
// length. Therefore, if the Prefix of the consolidated constraint is // less than the number of prefix columns, we try the unconsolidated // constraint. This allows us the optimizer to plan multi-column
[nit] us the optimizer -> the optimizer (or us! just pick one 🙂 )
pkg/sql/opt/invertedidx/inverted_index_expr.go, line 318 at r1 (raw file):
constraint = ic.UnconsolidatedConstraint() if constraint.Prefix(evalCtx) < prefixColumnCount { // If the all of the unconsolidated constraint spans do not have the
[nit] the all -> all
|
Thinking about this some more, I don't think we'd ever want to use a consolidate constraint in this instance. Any constraint that can be consolidated would be consolidated into one or more spans that doesn't have the same start and end key. So I think we only need to check the unconsolidated constraint here. Does anyone see a problem with that? |
rytaft
left a comment
There was a problem hiding this comment.
So I think we only need to check the unconsolidated constraint here. Does anyone see a problem with that?
Makes sense to me
Reviewable status:
cde4ca7 to
ea3af48
Compare
mgartner
left a comment
There was a problem hiding this comment.
Makes sense to me
Cool. PTAL!
Reviewable status:
pkg/sql/opt/invertedidx/inverted_index_expr.go, line 303 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] us the optimizer -> the optimizer (or us! just pick one 🙂 )
Done.
pkg/sql/opt/invertedidx/inverted_index_expr.go, line 318 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] the all -> all
Done.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2.
Reviewable status:
This commit allows for the prefix columns of multi-column inverted
indexes to be constrained to a contiguous range. For example, consider
the schema and query:
CREATE TABLE t (
k INT PRIMARY KEY,
i INT,
g GEOMETRY,
INVERTED INDEX (i, g)
)
SELECT k FROM t WHERE i IN (1, 2, 3) AND ST_CoveredBy(..., g)
Prior to this commit, a scan over the inverted index would not be
generated for this query.
Release note: None
ea3af48 to
8965de2
Compare
|
TFTRs! bors r+ |
|
Build succeeded: |
This commit allows for the prefix columns of multi-column inverted
indexes to be constrained to a contiguous range. For example, consider
the schema and query:
Prior to this commit, a scan over the inverted index would not be
generated for this query.
Release note: None