You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/30 13:56:19 UTC
[GitHub] [pinot] WangCHX opened a new pull request, #8620: Handle nested conversion of geo column
WangCHX opened a new pull request, #8620:
URL: https://github.com/apache/pinot/pull/8620
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1113998375
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8620](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (001caf6) into [master](https://codecov.io/gh/apache/pinot/commit/d282e455234ea3964d24235c5e45ec4d4d275a4f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d282e45) will **decrease** coverage by `56.37%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #8620 +/- ##
=============================================
- Coverage 70.43% 14.06% -56.38%
+ Complexity 4375 84 -4291
=============================================
Files 1697 1659 -38
Lines 89421 87671 -1750
Branches 13548 13361 -187
=============================================
- Hits 62988 12331 -50657
- Misses 21997 74415 +52418
+ Partials 4436 925 -3511
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.06% <0.00%> (-0.06%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...perator/filter/H3InclusionIndexFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvSDNJbmNsdXNpb25JbmRleEZpbHRlck9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-83.93%)` | :arrow_down: |
| [...ot/core/operator/filter/H3IndexFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvSDNJbmRleEZpbHRlck9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-82.98%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/plan/FilterPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0ZpbHRlclBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-82.54%)` | :arrow_down: |
| [...ava/org/apache/pinot/core/util/GeoColumnUtils.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dlb0NvbHVtblV0aWxzLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1359 more](https://codecov.io/gh/apache/pinot/pull/8620/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d282e45...001caf6](https://codecov.io/gh/apache/pinot/pull/8620?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1116762409
> > > 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](https://docs.pinot.apache.org/configuration-reference/functions/stcontains) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on a diff in pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on code in PR #8620:
URL: https://github.com/apache/pinot/pull/8620#discussion_r862414264
##########
pinot-core/src/main/java/org/apache/pinot/core/util/GeoColumnUtils.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import java.util.List;
+import java.util.Optional;
+import org.apache.pinot.common.request.context.ExpressionContext;
+
+
+public final class GeoColumnUtils {
+
+ private GeoColumnUtils() {
+ }
+
+ public static Optional<String> getColumnFromArgument(ExpressionContext argument) {
Review Comment:
got it. added comments and also handled multiple level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1120563912
doc: https://github.com/pinot-contrib/pinot-docs/pull/92
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1117709318
> > 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118872760
> > 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1116742625
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118067356
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1116824711
@yupeng9 Can we apply `ST_Contains` to the geography objects? I think it should work on both geometry and geography
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118044178
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118895319
I'm fine with it. @WangCHX Please highlight this and put a warning in the user doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on a diff in pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on code in PR #8620:
URL: https://github.com/apache/pinot/pull/8620#discussion_r862370012
##########
pinot-core/src/main/java/org/apache/pinot/core/util/GeoColumnUtils.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import java.util.List;
+import java.util.Optional;
+import org.apache.pinot.common.request.context.ExpressionContext;
+
+
+public final class GeoColumnUtils {
+
+ private GeoColumnUtils() {
+ }
+
+ public static Optional<String> getColumnFromArgument(ExpressionContext argument) {
Review Comment:
add some comments
also, i think this handles first level func only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on a diff in pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on code in PR #8620:
URL: https://github.com/apache/pinot/pull/8620#discussion_r862414264
##########
pinot-core/src/main/java/org/apache/pinot/core/util/GeoColumnUtils.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import java.util.List;
+import java.util.Optional;
+import org.apache.pinot.common.request.context.ExpressionContext;
+
+
+public final class GeoColumnUtils {
+
+ private GeoColumnUtils() {
+ }
+
+ public static Optional<String> getColumnFromArgument(ExpressionContext argument) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118881563
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1116753483
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on a diff in pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on code in PR #8620:
URL: https://github.com/apache/pinot/pull/8620#discussion_r863891845
##########
pinot-core/src/main/java/org/apache/pinot/core/util/GeoColumnUtils.java:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.util;
+
+import java.util.List;
+import java.util.Optional;
+import org.apache.pinot.common.request.context.ExpressionContext;
+
+
+public final class GeoColumnUtils {
+
+ private GeoColumnUtils() {
+ }
+
+ public static Optional<String> getColumnFromArgument(ExpressionContext argument) {
Review Comment:
Thanks, it's possible for other nested functions such as some geometry manipulation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1117671074
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118885053
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1119253128
> I'm fine with it. @WangCHX Please highlight this and put a warning in the user doc
sure. will add that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #8620: Remove geometry check for st_contain and st_within
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8620:
URL: https://github.com/apache/pinot/pull/8620
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1116220449
plz take a look at the tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118016771
> > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] yupeng9 commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
yupeng9 commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118020075
> > > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1117690198
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX commented on pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX commented on PR #8620:
URL: https://github.com/apache/pinot/pull/8620#issuecomment-1118067608
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] WangCHX closed pull request #8620: Handle nested conversion of geo column
Posted by GitBox <gi...@apache.org>.
WangCHX closed pull request #8620: Handle nested conversion of geo column
URL: https://github.com/apache/pinot/pull/8620
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org