opt: eliminate unnecessary index join inside project#53586
opt: eliminate unnecessary index join inside project#53586craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
I can't think of a case outside of partial indexes (and inverted indexes, see TODO in diff) where this rule would fire, so I think that makes it low-risk. However, if there are cases where this rule fires and we consider it not low-risk, this can wait until a future release. |
da65da7 to
feefd01
Compare
|
Note for me: Rebase and fix |
rytaft
left a comment
There was a problem hiding this comment.
It would be cool if we could also have a similar rule that would apply to LookupJoins that are acting like an IndexJoin (or did @DrewKimball already make a rule to eliminate those?). I don't think we should do that now during the stability period, but maybe add a TODO somewhere?
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
|
This fires for unnecessary index joins with inverted indexes (as seen in some tests). It looks correct though, because (since very recently) we use a new ColumnID for the index "column". I am ok with this going in (it's a low risk improvement for both existing and new functionality) , but we may want to add some more specific tests with inverted indexes (and I'm sure there's a TODO about this somewhere that we can remove). |
@RaduBerinde Can you explain what you mean by this? Why wouldn't this have been safe before this change? |
|
In principle the scan of an inverted index can return the inverted index key as a column. I am not sure this happens currently for JSON but it can definitely happen for geospatial. Until my recent PR, this "column" had the same ColumnID as the "real" table column. So you could conceivably have an index join that looks unnecessary, but that's actually replacing the incomplete inverted key value with the real column value. I am not 100% sure this would occur in practice currently so maybe there's nothing to do here. I'd just ensure that we have tests like |
feefd01 to
69f3c90
Compare
mgartner
left a comment
There was a problem hiding this comment.
It would be cool if we could also have a similar rule that would apply to LookupJoins that are acting like an IndexJoin (or did @DrewKimball already make a rule to eliminate those?). I don't think we should do that now during the stability period, but maybe add a TODO somewhere?
@rytaft do you have an example? I couldn't find any tests that show what you're describing.
In principle the scan of an inverted index can return the inverted index key as a column. I am not sure this happens currently for JSON but it can definitely happen for geospatial. Until my recent PR, this "column" had the same ColumnID as the "real" table column. So you could conceivably have an index join that looks unnecessary, but that's actually replacing the incomplete inverted key value with the real column value. I am not 100% sure this would occur in practice currently so maybe there's nothing to do here. I'd just ensure that we have tests like SELECT j FROM t WHERE j <@ ... and those still do an index join.
Got it. I've added a test with a comment to make this case explicit. I don't think it was possible because inverted index scans seem to "know" that they can only output the primary key columns, and not the column they index.
Reviewable status:
Hmm I thought that maybe there might be a case from the |
69f3c90 to
0e611f9
Compare
I'll keep it in mind and if I come across a case we can add support for it. Without an existing test case, I'm not sure where I would put a TODO, so I'm inclined to forgo it for now. |
|
Sounds good to me |
This commit adds a new rule, EliminateIndexJoinInsideProject, that removes an IndexJoin inside a Project when the IndexJoin's input produces all the columns needed by the Project's passthrough and projection columns. See the comments for the new rule for a detailed example. Release justification: This is a low-risk update to new functionality, partial indexes. Release note: None
0e611f9 to
ae2a9e0
Compare
Done. |
|
@RaduBerinde Do you sign-off on this? |
|
bors r+ |
|
Build succeeded: |
This commit adds a new rule, EliminateIndexJoinInsideProject, that
removes an IndexJoin inside a Project when the IndexJoin's input
produces all the columns needed by the Project's passthrough and
projection columns. See the comments for the new rule for a detailed
example.
Fixes #51018
Release justification: This is a low-risk update to new functionality,
partial indexes.
Release note: None