opt: fix FoldEqZeroSTDistance with use_spheroid argument#67340
opt: fix FoldEqZeroSTDistance with use_spheroid argument#67340craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
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.
13deb11 to
3542ffe
Compare
otan
left a comment
There was a problem hiding this comment.
mind backporting this all the way to v20.2?
Will do. |
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2)
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 2 of 0 LGTMs obtained (waiting on @mgartner)
|
TFTRs! bors r+ |
|
Build failed (retrying...): |
|
a prime candidate for https://github.com/cockroachlabs/blathers-bot/pull/45 ! |
This comment has been minimized.
This comment has been minimized.
|
bors r- 🚨 bors is busted 🚨 |
|
Canceled. |
|
bors r+ |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
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. |
The
FoldEqZeroSTDistancenormalization rule normalizes expressions inthe form
st_distance(a, b) = 0tost_intersects(a, b). Thisnormalization rule is important because it allows an inverted index to
be used for more queries.
Previously,
FoldEqZeroSTDistancedid not handle the third argument ofst_distancefor the geography overload,use_spheroid. The optimizerpanicked when a user supplied this third argument because it attempted
to find an
st_intersectsoverload with the three arguments and no suchoverload exists.
In addition to the panic, this rule was incorrectly firing in cases
where it shouldn't have.
From the
ST_DistancePostGIS docs:From the
ST_IntersectsPostGIS docs:In summary,
st_distancecalculates on a spheroid by default, and onlyon a sphere if
use_spheroidis explicitly false.st_intersectscalculates on the sphere always. Therefore, this rule can only apply for
geography types if
use_spheroid=falseis explicitly passed.This commit ensures that
FoldEqZeroSTDistanceonly matchesst_distancefunctions with geography arguments ifuse_spheroid=falseis included in the function call. The panic is fixed by stripping the
use_spheroidargument from the list of arguments used to lookup thest_intersectsoverload.Fixes #67235
Release note (bug fix): Two bugs have been fixed which affected
geospatial queries with the
st_distancefunction. The first causederrors 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) = 0wheng1andg2are geographies tost_instersects(g1, g2). This is not a validtransformation because
st_distancemakes spheroid-based calculationsby default while
st_intersectsonly makes sphere-based calculations.