opt: fix panic in GenerateInvertedIndexScans #60598
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)
pkg/sql/opt/invertedidx/geo.go, line 528 at r1 (raw file):
// spanExpr is the result of evaluating the geospatial relationship // represented by this geoInvertedExpr. It is nil prior to evaluation. spanExpr inverted.Expression
[nit] maybe change the name of this variable to invertedExpr now
pkg/sql/opt/invertedidx/geo.go, line 807 at r1 (raw file):
if d, ok := nonIndexParam.(tree.Datum); ok { spanExpr = g.getSpanExpr(evalCtx.Ctx(), d, additionalParams, relationship, g.indexConfig)
[nit] extra blank line
mgartner
force-pushed
the
59702-fix-nil-span-expr
branch
from
9dcf77a
to
e0c3708
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/invertedidx/geo.go, line 528 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] maybe change the name of this variable to
invertedExprnow
Done.
pkg/sql/opt/invertedidx/geo.go, line 807 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] extra blank line
Done.
There was a problem hiding this comment.
Like I mentioned in the other PR, if you can find an inverted join test case similar to the inverted filter case, that would be good to add (guessing it's just a matter of putting the constant argument you passed to st_intersects into a table and joining with it). Since the panic won't show up until execution time, it will probably need to be a logictest.
Reviewed 2 of 2 files at r2.
Reviewable status:
pkg/sql/opt/invertedidx/geo.go, line 526 at r2 (raw file):
additionalParams []tree.Datum // spanExpr is the result of evaluating the geospatial relationship
spanExpr -> invertedExpr
mgartner
force-pushed
the
59702-fix-nil-span-expr
branch
2 times, most recently
from
78725a2
to
92e124a
Compare
There was a problem hiding this comment.
I found a reproduction for inverted joins that panics during optimization. 😄
Reviewable status:
pkg/sql/opt/invertedidx/geo.go, line 526 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
spanExpr -> invertedExpr
Done.
mgartner
changed the title
There was a problem hiding this comment.
I found a reproduction for inverted joins that panics during optimization. 😄
Nevermind... I was wrong.
Reviewable status:
There was a problem hiding this comment.
Lol oh well. Does the new test case you added panic during execution (before the change you made)?
Reviewed 2 of 2 files at r3.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 5530 at r3 (raw file):
SELECT * FROM small JOIN t59702 ON st_intersects(t59702.g, st_buffer(st_makepoint(1, 1)::GEOGRAPHY, 0)) AND small.m = t59702.k
Is this testing anything? It's just creating a lookup join, not an inverted join or inverted filter
mgartner
changed the title
mgartner
force-pushed
the
59702-fix-nil-span-expr
branch
from
92e124a
to
e608216
Compare
There was a problem hiding this comment.
Lol oh well. Does the new test case you added panic during execution (before the change you made)?
Yes, but only because the GenerateInvertedScans rule panicked, not the GenerateInvertedJoins rule. I've remove it.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/join, line 5530 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Is this testing anything? It's just creating a lookup join, not an inverted join or inverted filter
I've removed it.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4.
Reviewable status:
This commit fixes a nil-pointer exception caused when empty index spans were generated from a filter on an inverted column. Fixes cockroachdb#59702 Release note (bug fix): A bug has been fixed that caused errors for some queries on tables with GEOMETRY or GEOGRAPHY inverted indexes with filters containing shapes with zero area.
mgartner
force-pushed
the
59702-fix-nil-span-expr
branch
from
e608216
to
da02c10
Compare
|
TFTRs! bors r+ |
|
Build succeeded: |
This commit fixes a nil-pointer exception caused when empty index spans
were generated from a filter on an inverted column.
Fixes #59702
Release note (bug fix): A bug has been fixed that caused errors for some
queries on tables with GEOMETRY or GEOGRAPHY inverted indexes with
filters containing shapes with zero area.