Skip to content

make st_within use spatial indexes #1641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 8, 2023
Merged

make st_within use spatial indexes #1641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
55386bd
have st_within use spatial indexes
Mar 7, 2023
9e7d943
[ga-format-pr] Run ./format_repo.sh to fix formatting
jcor11599 Mar 7, 2023
bbd37a0
remove unused function
Mar 7, 2023
16a14eb
feedback
Mar 8, 2023
6a13976
[ga-format-pr] Run ./format_repo.sh to fix formatting
jcor11599 Mar 8, 2023
7e3eb15
empty set
Mar 8, 2023
736c5f4
merge
Mar 8, 2023
7df761b
[ga-format-pr] Run ./format_repo.sh to fix formatting
jcor11599 Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions enginetest/spatial_index_tests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,39 @@ var SpatialIndexTests = []SpatialIndexPlanTest{
},
},
},
{
name: "filter point table with st_within",
setup: []string{
"create table point_tbl(p point not null srid 0, spatial index (p))",
"insert into point_tbl values (point(0,0)), (point(1,1)), (point(2,2))",
"create table point_pk_tbl(i int primary key, p point not null srid 0, spatial index (p))",
"insert into point_pk_tbl values (0, point(0,0)), (1, point(1,1)), (2, point(2,2))",
},
tests: []SpatialIndexPlanTestAssertion{
{
q: "select p from point_tbl where st_within(p, point(0,0))",
Copy link
Contributor

@max-hoffman max-hoffman Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming, this validates that point_tbl is indexed? If someone breaks this in the future this will error to notify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a test for null point? null value input into filter? can the index be a primary? the is a keyless table, maybe a pk table also? query that returns data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, evalSpatialIndexPlanTest should check that the plan produced contains both a Filter and a IndexAccess

https://github.com/dolthub/go-mysql-server/pull/1641/files#diff-c78bcec60f7605673e947656af38792e90f53fdacd97c7def3e2142a41ae581dR352

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add tests for primary key table and a null point.

exp: []sql.Row{
{types.Point{X: 0, Y: 0}},
},
},
{
noIdx: true,
q: "select p from point_tbl where st_within(p, null)",
exp: []sql.Row{},
},
{
q: "select i, p from point_pk_tbl where st_within(p, point(0,0))",
exp: []sql.Row{
{0, types.Point{X: 0, Y: 0}},
},
},
{
noIdx: true,
q: "select i, p from point_pk_tbl where st_within(p, null)",
exp: []sql.Row{},
},
},
},
}

func TestSpatialIndexPlans(t *testing.T, harness Harness) {
Expand Down
16 changes: 14 additions & 2 deletions sql/analyzer/indexes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package analyzer

import (
"fmt"

"github.com/dolthub/go-mysql-server/sql"
"github.com/dolthub/go-mysql-server/sql/expression"
"github.com/dolthub/go-mysql-server/sql/expression/function/spatial"
Expand Down Expand Up @@ -269,7 +271,7 @@ func getIndexes(
// TODO (james): add all other spatial index supported functions here
// TODO: make generalizable to all functions?
switch e := e.(type) {
case *spatial.Intersects:
case *spatial.Intersects, *spatial.Within:
// don't pushdown functions with bindvars
if exprHasBindVar(e) {
return nil, nil
Expand All @@ -281,8 +283,14 @@ func getIndexes(
return nil, nil
}

// Assume these are all BinaryExpression with exactly two children
children := e.Children()
if len(children) != 2 {
return nil, fmt.Errorf("st function is not a binary expression")
}

// Put GetField on the left
left, right := e.BinaryExpression.Left, e.BinaryExpression.Right
left, right := children[0], children[1]
if _, ok := right.(*expression.GetField); ok {
left, right = right, left
}
Expand All @@ -292,6 +300,10 @@ func getIndexes(
return nil, err
}

if value == nil {
return nil, nil
}

g, ok := value.(types.GeometryValue)
if !ok {
return nil, sql.ErrInvalidGISData.New()
Expand Down