Skip to content

opt: fix FoldEqZeroSTDistance with use_spheroid argument#67340

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:67235-st-distance-panic
Jul 9, 2021
Merged

opt: fix FoldEqZeroSTDistance with use_spheroid argument#67340
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:67235-st-distance-panic

Conversation

@mgartner
Copy link
Copy Markdown
Contributor

@mgartner mgartner commented Jul 7, 2021

The FoldEqZeroSTDistance normalization rule normalizes expressions in
the form st_distance(a, b) = 0 to st_intersects(a, b). This
normalization rule is important because it allows an inverted index to
be used for more queries.

Previously, FoldEqZeroSTDistance did not handle the third argument of
st_distance for the geography overload, use_spheroid. The optimizer
panicked when a user supplied this third argument because it attempted
to find an st_intersects overload with the three arguments and no such
overload exists.

In addition to the panic, this rule was incorrectly firing in cases
where it shouldn't have.

From the ST_Distance PostGIS docs:

For geography types defaults to return the minimum geodesic distance
between two geographies in meters, compute on the spheroid determined
by the SRID. If use_spheroid is false, a faster spherical calculation
is used.

From the ST_Intersects PostGIS docs:

For geography, this function has a distance tolerance of about 0.00001
meters and uses the sphere rather than spheroid calculation.

In summary, st_distance calculates on a spheroid by default, and only
on a sphere if use_spheroid is explicitly false. st_intersects
calculates on the sphere always. Therefore, this rule can only apply for
geography types if use_spheroid=false is explicitly passed.

This commit ensures that FoldEqZeroSTDistance only matches
st_distance functions with geography arguments if use_spheroid=false
is included in the function call. The panic is fixed by stripping the
use_spheroid argument from the list of arguments used to lookup the
st_intersects overload.

Fixes #67235

Release note (bug fix): Two bugs have been fixed which affected
geospatial queries with the st_distance function. The first caused
errors for filters of the form st_distance(g1, g2, use_spheroid) = 0.
The second could cause incorrect results in some cases. It incorrectly
transformed filters in the form st_distance(g1, g2) = 0 when g1 and
g2 are geographies to st_instersects(g1, g2). This is not a valid
transformation because st_distance makes spheroid-based calculations
by default while st_intersects only makes sphere-based calculations.

@mgartner mgartner requested review from a team, michae2, otan and rytaft July 7, 2021 23:17
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

The `FoldEqZeroSTDistance` normalization rule normalizes expressions in
the form `st_distance(a, b) = 0` to `st_intersects(a, b)`. This
normalization rule is important because it allows an inverted index to
be used for more queries.

Previously, `FoldEqZeroSTDistance` did not handle the third argument of
`st_distance` for the geography overload, `use_spheroid`. The optimizer
panicked when a user supplied this third argument because it attempted
to find an `st_intersects` overload with the three arguments and no such
overload exists.

In addition to the panic, this rule was incorrectly firing in cases
where it shouldn't have.

From the [`ST_Distance` PostGIS docs](https://postgis.net/docs/ST_Distance.html):

> For geography types defaults to return the minimum geodesic distance
> between two geographies in meters, compute on the spheroid determined
> by the SRID. If use_spheroid is false, a faster spherical calculation
> is used.

From the [`ST_Intersects` PostGIS docs](https://postgis.net/docs/ST_Intersects.html):

> For geography, this function has a distance tolerance of about 0.00001
> meters and uses the sphere rather than spheroid calculation.

In summary, `st_distance` calculates on a spheroid by default, and only
on a sphere if `use_spheroid` is explicitly false. `st_intersects`
calculates on the sphere always. Therefore, this rule can only apply for
geography types if `use_spheroid=false` is explicitly passed.

This commit ensures that `FoldEqZeroSTDistance` only matches
`st_distance` functions with geography arguments if `use_spheroid=false`
is included in the function call. The panic is fixed by stripping the
`use_spheroid` argument from the list of arguments used to lookup the
`st_intersects` overload.

Fixes cockroachdb#67235

Release note (bug fix): Two bugs have been fixed which affected
geospatial queries with the `st_distance` function. The first caused
errors for filters of the form `st_distance(g1, g2, use_spheroid) = 0`.
The second could cause incorrect results in some cases. It incorrectly
transformed filters in the form `st_distance(g1, g2) = 0` when `g1` and
`g2` are geographies to `st_instersects(g1, g2)`. This is not a valid
transformation because `st_distance` makes spheroid-based calculations
by default while `st_intersects` only makes sphere-based calculations.
@mgartner mgartner force-pushed the 67235-st-distance-panic branch from 13deb11 to 3542ffe Compare July 7, 2021 23:19
Copy link
Copy Markdown
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

mind backporting this all the way to v20.2?

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jul 7, 2021

mind backporting this all the way to v20.2?

Will do.

Copy link
Copy Markdown
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

Copy link
Copy Markdown
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jul 8, 2021

TFTRs!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 8, 2021

Build failed (retrying...):

@otan
Copy link
Copy Markdown
Contributor

otan commented Jul 8, 2021

a prime candidate for https://github.com/cockroachlabs/blathers-bot/pull/45 !

@blathers-crl

This comment has been minimized.

@otan
Copy link
Copy Markdown
Contributor

otan commented Jul 9, 2021

bors r-

🚨 bors is busted 🚨

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 9, 2021

Canceled.

@otan
Copy link
Copy Markdown
Contributor

otan commented Jul 9, 2021

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 9, 2021

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 9, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 9, 2021

Build succeeded:

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Jul 9, 2021

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating backport branch refs/heads/blathers/backport-release-21.1-67340: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 21.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@mgartner mgartner deleted the 67235-st-distance-panic branch July 9, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: st_distance = 0 optimiser normalisation panics

6 participants