Remove geometry check for st_contain and st_within #8620

WangCHX · 2022-04-30T13:56:17Z

Instructions:

The PR has to be tagged with at least one of the following labels (*):
1. enhancement

codecov-commenter · 2022-04-30T14:31:45Z

Codecov Report

Merging #8620 (59f1b47) into master (b5690a7) will decrease coverage by 41.10%.
The diff coverage is 8.16%.

@@              Coverage Diff              @@
##             master    #8620       +/-   ##
=============================================
- Coverage     70.47%   29.36%   -41.11%     
=============================================
  Files          1703     1692       -11     
  Lines         89569    89238      -331     
  Branches      13564    13532       -32     
=============================================
- Hits          63123    26204    -36919     
- Misses        21990    60602    +38612     
+ Partials       4456     2432     -2024

Flag	Coverage Δ
integration1	`27.30% <8.16%> (+0.12%)`	⬆️
integration2	`25.48% <8.16%> (-0.03%)`	⬇️
unittests1	`?`
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...perator/filter/H3InclusionIndexFilterOperator.java	`0.00% <0.00%> (-83.93%)`	⬇️
...ot/core/operator/filter/H3IndexFilterOperator.java	`0.00% <0.00%> (-82.98%)`	⬇️
...ava/org/apache/pinot/core/util/GeoColumnUtils.java	`0.00% <0.00%> (ø)`
.../org/apache/pinot/segment/local/utils/H3Utils.java	`0.00% <0.00%> (-40.39%)`	⬇️
...ava/org/apache/pinot/core/plan/FilterPlanNode.java	`55.03% <17.39%> (-27.51%)`	⬇️
...in/java/org/apache/pinot/spi/utils/StringUtil.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1173 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5690a7...59f1b47. Read the comment docs.

yupeng9 · 2022-04-30T15:54:19Z

pinot-core/src/main/java/org/apache/pinot/core/util/GeoColumnUtils.java

add some comments

also, i think this handles first level func only

got it. added comments and also handled multiple level.
just was not sure multiple level is a common thing.
like

toGeometry(toSphericalGeography(toGeometry(%s)))

looks like unnecessary.

Thanks, it's possible for other nested functions such as some geometry manipulation.

yupeng9 · 2022-05-03T15:16:48Z

plz take a look at the tests

Jackie-Jiang

This is not the correct fix. The filter operator relies on the H3 index on the column. If the column is within an expression, the result will be wrong. E.g. ST_DISTANCE(geoCol, 'point') is not the same as ST_DISTANCE(myFunc(geoCol), 'point'), and we cannot use the same index because the index is not applied to the myFunc(geoCol)

WangCHX · 2022-05-03T23:09:07Z

This is not the correct fix. The filter operator relies on the H3 index on the column. If the column is within an expression, the result will be wrong. E.g. ST_DISTANCE(geoCol, 'point') is not the same as ST_DISTANCE(myFunc(geoCol), 'point'), and we cannot use the same index because the index is not applied to the myFunc(geoCol)

is it ok for me to add constraints that the function only can be toSphericalGeography and toGeometry right now?

Jackie-Jiang · 2022-05-03T23:23:03Z

This is not the correct fix. The filter operator relies on the H3 index on the column. If the column is within an expression, the result will be wrong. E.g. ST_DISTANCE(geoCol, 'point') is not the same as ST_DISTANCE(myFunc(geoCol), 'point'), and we cannot use the same index because the index is not applied to the myFunc(geoCol)

is it ok for me to add constraints that the function only can be toSphericalGeography and toGeometry right now?

Why do you need to do toSphericalGeography or toGeometry on a column with H3 index? IIUC, H3 index implies the underlying points are geography, and ST_DISTANCE(toGeometry(geoCol), 'point') should not use H3 index.

WangCHX · 2022-05-03T23:38:02Z

This is not the correct fix. The filter operator relies on the H3 index on the column. If the column is within an expression, the result will be wrong. E.g. ST_DISTANCE(geoCol, 'point') is not the same as ST_DISTANCE(myFunc(geoCol), 'point'), and we cannot use the same index because the index is not applied to the myFunc(geoCol)

is it ok for me to add constraints that the function only can be toSphericalGeography and toGeometry right now?

Why do you need to do toSphericalGeography or toGeometry on a column with H3 index? IIUC, H3 index implies the underlying points are geography, and ST_DISTANCE(toGeometry(geoCol), 'point') should not use H3 index.

I think the underlying is a point. H3 index only take coordinate. so it can be applied to both geography and geometry. Also ST_Contains only can be applied to Geometry. If i want to use ST_DISTANCE and ST_Contains at the same time, seems I need to create two columns.

Jackie-Jiang · 2022-05-04T00:36:37Z

@yupeng9 Can we apply ST_Contains to the geography objects? I think it should work on both geometry and geography

yupeng9 · 2022-05-04T18:31:20Z

current implementation of ST_Contains is for Geometry only
https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java

Jackie-Jiang · 2022-05-04T18:52:25Z

current implementation of ST_Contains is for Geometry only https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java

@yupeng9 Understood, but why does it not apply to the geography objects?

yupeng9 · 2022-05-04T19:06:12Z

current implementation of ST_Contains is for Geometry only https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java

@yupeng9 Understood, but why does it not apply to the geography objects?

Different implementation, and I didn't scope it in the inital impl

Jackie-Jiang · 2022-05-04T23:14:01Z

current implementation of ST_Contains is for Geometry only https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java

@yupeng9 Understood, but why does it not apply to the geography objects?

Different implementation, and I didn't scope it in the inital impl

@yupeng9 Is H3 index always based on the geography objects? If so, we cannot use H3 index for ST_Contains because that will give wrong result (mixing geometry calculation with geography calculation).
Another option is to allow using geometry contains on geography objects because they should give close estimation on small areas. It won't work for large areas though.

yupeng9 · 2022-05-04T23:20:23Z

current implementation of ST_Contains is for Geometry only https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java

@yupeng9 Understood, but why does it not apply to the geography objects?

Different implementation, and I didn't scope it in the inital impl

@yupeng9 Is H3 index always based on the geography objects? If so, we cannot use H3 index for ST_Contains because that will give wrong result (mixing geometry calculation with geography calculation). Another option is to allow using geometry contains on geography objects because they should give close estimation on small areas. It won't work for large areas though.

Yes, H3 is always based on geography. And yes, we can do approximation for small area.

Jackie-Jiang · 2022-05-05T00:18:38Z

Then my suggestion is to remove the geometry check in the StContainsFunction, and we should document the behavior of using geometry contains for geography objects, which can give good estimation on small areas. We should also add a TODO to support accurate contains on geography objects in the future.

WangCHX · 2022-05-05T00:55:11Z

Then my suggestion is to remove the geometry check in the StContainsFunction, and we should document the behavior of using geometry contains for geography objects, which can give good estimation on small areas. We should also add a TODO to support accurate contains on geography objects in the future.

thanks Jackie and Yupeng. just curious why it is not accurate? My use case is checking all the coordinates inside a city polygon (like SF). is it small area or big area for a city like SF?

* Adding DML definition and parse sql * Use MinionClient for execute async task * Add integration tests

Jackie-Jiang · 2022-05-05T17:42:21Z

Then my suggestion is to remove the geometry check in the StContainsFunction, and we should document the behavior of using geometry contains for geography objects, which can give good estimation on small areas. We should also add a TODO to support accurate contains on geography objects in the future.

thanks Jackie and Yupeng. just curious why it is not accurate? My use case is checking all the coordinates inside a city polygon (like SF). is it small area or big area for a city like SF?

IMO a single city on the earth sphere can be considered small area.
I can see one exception area around longitude of -180/180, where geography and geometry will give very different result (in geography, -179.9 to 179.9 is a short distance, but in geometry that is a whole circle of the sphere). In other areas treating geography as geometry should give close approximation.
@yupeng9 What do you think if we allow ST_Contains and ST_Within on geography but use the geometry algorithm?

yupeng9 · 2022-05-05T17:53:26Z

@Jackie-Jiang is spot on. There are corner cases around the boundary, and thats the risk you have face with the approximation. Alternatively, you can explore the ST_Contains implementation for geography and add the proper impl to Pinot

Jackie-Jiang · 2022-05-05T17:57:36Z

I think we can merge this and update our documentation to warn about the behavior so that H3 index can be used for now. The geography support can be added in a separate PR. @yupeng9 wdyt?

yupeng9 · 2022-05-05T18:08:38Z

I'm fine with it. @WangCHX Please highlight this and put a warning in the user doc

WangCHX · 2022-05-06T04:37:12Z

I'm fine with it. @WangCHX Please highlight this and put a warning in the user doc

sure. will add that.

WangCHX · 2022-05-09T02:15:59Z

doc: pinot-contrib/pinot-docs#92

yupeng9 reviewed Apr 30, 2022

View reviewed changes

yupeng9 approved these changes May 3, 2022

View reviewed changes

Jackie-Jiang added the enhancement label May 3, 2022

Jackie-Jiang requested changes May 3, 2022

View reviewed changes

WangCHX closed this May 5, 2022

WangCHX reopened this May 5, 2022

WangCHX force-pushed the cwang/nest_st branch from d7c54ee to 99b3301 Compare May 5, 2022 13:19

xiangfu0 and others added 4 commits May 5, 2022 21:22

Adding DML definition and parse SQL InsertFile (apache#8557)

86dcc7d

* Adding DML definition and parse sql * Use MinionClient for execute async task * Add integration tests

small optimization on fully contain

e5115ad

remove geometry check on st contain and st within

f2b0f5b

Merge branch 'master' into cwang/nest_st

180fc82

WangCHX force-pushed the cwang/nest_st branch from 99b3301 to 180fc82 Compare May 5, 2022 13:22

WangCHX changed the title ~~Handle nested conversion of geo column~~ Remove geometry check for st_contain and st_within May 5, 2022

Jackie-Jiang approved these changes May 5, 2022

View reviewed changes

Jackie-Jiang merged commit 60e2706 into apache:master May 5, 2022

WangCHX deleted the cwang/nest_st branch May 6, 2022 04:37

yashmayya mentioned this pull request Jun 21, 2024

Add some missing geospatial scalar functions to support use in intermediate stages with the v2 query engine #13457

Merged

Remove geometry check for st_contain and st_within #8620

Remove geometry check for st_contain and st_within #8620

Uh oh!

Conversation

WangCHX commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yupeng9 Apr 30, 2022

Choose a reason for hiding this comment

Uh oh!

WangCHX May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yupeng9 May 3, 2022

Choose a reason for hiding this comment

Uh oh!

yupeng9 commented May 3, 2022

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

WangCHX commented May 3, 2022

Uh oh!

Jackie-Jiang commented May 3, 2022

Uh oh!

WangCHX commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang commented May 4, 2022

Uh oh!

yupeng9 commented May 4, 2022

Uh oh!

Jackie-Jiang commented May 4, 2022

Uh oh!

yupeng9 commented May 4, 2022

Uh oh!

Jackie-Jiang commented May 4, 2022

Uh oh!

yupeng9 commented May 4, 2022

Uh oh!

Jackie-Jiang commented May 5, 2022

Uh oh!

WangCHX commented May 5, 2022

Uh oh!

Jackie-Jiang commented May 5, 2022

Uh oh!

yupeng9 commented May 5, 2022

Uh oh!

Jackie-Jiang commented May 5, 2022

Uh oh!

yupeng9 commented May 5, 2022

Uh oh!

WangCHX commented May 6, 2022

Uh oh!

WangCHX commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WangCHX commented Apr 30, 2022 •

edited

Loading

codecov-commenter commented Apr 30, 2022 •

edited

Loading

WangCHX May 1, 2022 •

edited

Loading

WangCHX commented May 3, 2022 •

edited

Loading