opt: add indexed var for inverted column when execbuilding inverted joins#50754
opt: add indexed var for inverted column when execbuilding inverted joins#50754craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Hold off on reviewing this for now -- I'm testing it with #50709 to see if I can identify any other issues. |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @sumeerbhola)
pkg/sql/opt/exec/execbuilder/relational.go, line 1403 at r1 (raw file):
lookupOrdinals, lookupColMap := b.getColumns(lookupCols, join.Table) allCols := joinOutputMap(input.outputCols, lookupColMap)
I think this will include this inverted column in allCols which is the output of this join, which isn't quite correct.
Are we going down the path of the invertedFilterer integration where we couldn't figure out how to do the col mappings in a way that the inverted column was provided in the input but not the output and needed the projection?
b0a4fe7 to
dd7e9da
Compare
|
pkg/sql/opt/exec/execbuilder/relational.go, line 1403 at r1 (raw file): Previously, sumeerbhola wrote…
Yea, just added the post projection here. PTAL. I tried to remove it from the outputCols directly, but it still showed up in the EXPLAIN, so I don't think it was working. |
|
pkg/sql/opt/exec/execbuilder/relational.go, line 1403 at r1 (raw file): Previously, rytaft (Rebecca Taft) wrote…
cc @RaduBerinde do you know if there is a better way to do this? |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/exec/execbuilder/relational.go, line 1403 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
cc @RaduBerinde do you know if there is a better way to do this?
Not sure I understand the issue.. ConstructInvertedJoin is documented as producing all the lookup columns, if that's the case, yes a projection is needed.
…oins This commit fixes an issue identified in cockroachdb#50709 in which the execbuilder code for building inverted joins was failing due to the lack of an indexed var for the column indexed by the inverted index. The indexed var is necessary since the inverted expression contains a reference to the inverted column. This commit fixes the issue by including the column in the indexed var helper. This commit does not include a release note since this issue was introduced a few days ago and has not been released. Release note: None
dd7e9da to
59286e4
Compare
|
pkg/sql/opt/exec/execbuilder/relational.go, line 1403 at r1 (raw file): Previously, RaduBerinde wrote…
The issue is that it should be possible for the The documentation for |
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 2 of 2 files at r2.
Reviewable status:
|
bors r+ |
Build failed |
|
Opened #50795 bors r+ |
Build succeeded |
This commit fixes an issue identified in #50709 in which the
execbuildercode for building inverted joins was failing due to the lack of an indexed
var for the column indexed by the inverted index. The indexed var is
necessary since the inverted expression contains a reference to the inverted
column. This commit fixes the issue by including the column in the indexed var
helper.
This commit does not include a release note since this issue was introduced
a few days ago and has not been released.
Release note: None