opt: add index acceleration support for ~ and && bounding box operators #53023
Conversation
|
This change is |
|
LGTM, but not fully read up on optimizer bits to point out anything major. |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @mjibson, @otan, and @rytaft)
pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial, line 223 at r1 (raw file):
query II SELECT lk, rk FROM ltable JOIN rtable@geom_index ON rtable.geom && ltable.geom1
can you add some tests here and for inverted_filterer where one of these is cast to a box2d before doing && or ~.
pkg/sql/opt/invertedidx/geo.go, line 348 at r1 (raw file):
t.Child(0).(opt.ScalarExpr), t.Child(1).(opt.ScalarExpr), } // Cast the arguments to type Geometry if needed (they may be type box2d).
same question as below about the cast failing.
pkg/sql/opt/invertedidx/geo.go, line 532 at r1 (raw file):
t.Child(0).(opt.ScalarExpr), t.Child(1).(opt.ScalarExpr), } // Cast the arguments to type Geometry if needed (they may be type box2d).
what if the argument is not box2d? How is that cast error handled?
pkg/sql/opt/xform/testdata/rules/join, line 3491 at r1 (raw file):
FROM nyc_census_blocks AS c JOIN nyc_neighborhoods@nyc_neighborhoods_geo_idx AS n ON c.geom::box2d ~ n.geom
can you add a test with
n.geom ~ c.geom::box2d
to show that it uses st_coveredby for the join.
pkg/sql/opt/xform/testdata/rules/select, line 2186 at r1 (raw file):
opt SELECT k FROM g WHERE geom ~ 'BOX(1 2, 3 4)'::box2d
this is the CoveredBy case for the index yes?
I'm surprised that this isn't resulting in any intersection in the inverted expression below. It may be because these tests use a cell covering that is very coarse, so maybe the inner covering for this Box was a single cell. Can you tweak that to confirm that the right thing is happening?
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/sql/logictest/testdata/logic_test/inverted_join_geospatial, line 223 at r1 (raw file):
Previously, sumeerbhola wrote…
can you add some tests here and for inverted_filterer where one of these is cast to a box2d before doing
&&or~.
Done.
pkg/sql/opt/invertedidx/geo.go, line 348 at r1 (raw file):
Previously, sumeerbhola wrote…
same question as below about the cast failing.
Done.
pkg/sql/opt/invertedidx/geo.go, line 532 at r1 (raw file):
Previously, sumeerbhola wrote…
what if the argument is not box2d? How is that cast error handled?
This works because the optimizer will not fold a cast if it would cause an error. If an argument is not either type geometry or box2d, that means this is not a bounding box operator, and therefore the checks inside constrainGeoIndexFromExpr will fail anyway.
However, I agree this is not obvious, and it seems like it only works by accident. I've cleaned it up a bit so hopefully it's clearer that it should work.
pkg/sql/opt/xform/testdata/rules/join, line 3491 at r1 (raw file):
Previously, sumeerbhola wrote…
can you add a test with
n.geom ~ c.geom::box2dto show that it uses
st_coveredbyfor the join.
Done.
pkg/sql/opt/xform/testdata/rules/select, line 2186 at r1 (raw file):
Previously, sumeerbhola wrote…
this is the CoveredBy case for the index yes?
I'm surprised that this isn't resulting in any intersection in the inverted expression below. It may be because these tests use a cell covering that is very coarse, so maybe the inner covering for this Box was a single cell. Can you tweak that to confirm that the right thing is happening?
Done.
|
pkg/sql/opt/invertedidx/geo.go, line 48 at r2 (raw file):
Would it make sense to have different memo expressions for the geo version of these operators? It might be safer in the case that normalization rules are added for I did something similar for the |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r2.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/invertedidx/geo.go, line 48 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Would it make sense to have different memo expressions for the geo version of these operators? It might be safer in the case that normalization rules are added for
*memo.RegMatchExpror*memo.OverlapsExprthat don't apply to the geo versions.I did something similar for the
IS NULLoperator here, FWIW: https://github.com/cockroachdb/cockroach/pull/48299/files#diff-15f17802e304627c3b80d68ac3394c4f
Good idea - Done.
There was a problem hiding this comment.
Are changes in execbuilder/scalar.go required to turn *memo.BBoxCoversExpr and *memo.BBoxIntersectsExpr back into their original tree representation in the case that a query does not use an inverted index?
Reviewed 6 of 9 files at r2.
Reviewable status:
pkg/sql/opt/ops/scalar.opt, line 475 at r3 (raw file):
[Scalar, Bool, Comparison] define BBoxCovers {
[nit] add a brief comment here explaining that these are ~ and && expressions for Geometry or Box2d types.
pkg/sql/opt/optbuilder/testdata/scalar, line 1382 at r3 (raw file):
overlaps [type=bool] ├── variable: "@1":1 [type=int[]] └── variable: "@2":2 [type=int[]]
[nit] add a test for when one child is a geometry/box2d and the other is not
This commit adds index acceleration support for the bounding box comparison operators, ~ and &&. It maps ~ to Covers and && to Intersects. Release note (performance improvement): The ~ and && geospatial bounding box operations can now benefit from index acceleration if one of the operands is an indexed geometry column.
There was a problem hiding this comment.
TFTRs!
Are changes in execbuilder/scalar.go required to turn *memo.BBoxCoversExpr and *memo.BBoxIntersectsExpr back into their original tree representation in the case that a query does not use an inverted index?
Nope, ComparisonOpReverseMap in operator.go does that.
Reviewable status:
pkg/sql/opt/ops/scalar.opt, line 475 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
[nit] add a brief comment here explaining that these are
~and&&expressions for Geometry or Box2d types.
Done.
pkg/sql/opt/optbuilder/testdata/scalar, line 1382 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
[nit] add a test for when one child is a geometry/box2d and the other is not
Done.
There was a problem hiding this comment.
Nope, ComparisonOpReverseMap in operator.go does that.
Ahh, very good.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
This commit adds index acceleration support for the bounding box comparison
operators,
~and&&. It maps~to Covers and&&to Intersects.Release note (performance improvement): The ~ and && geospatial bounding
box operations can now benefit from index acceleration if one of the
operands is an indexed geometry column.