You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Peter Rozsa (Code Review)" <ge...@cloudera.org> on 2023/01/17 14:25:10 UTC
[Impala-ASF-CR] WIP IMPALA-11745
Peter Rozsa has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19425
Change subject: WIP IMPALA-11745
......................................................................
WIP IMPALA-11745
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/catalog/geospatial/STMultiPointWrapper.java
A fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java
A fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java
A fe/src/main/java/org/apache/impala/catalog/geospatial/StLineStringWrapper.java
A fe/src/main/java/org/apache/impala/catalog/geospatial/StPolygonWrapper.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
M testdata/datasets/functional/functional_schema_template.sql
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_geospatial_udfs.py
M tests/query_test/test_udfs.py
33 files changed, 3,619 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/2
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12189/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 14:55:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12203/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jan 2023 13:44:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 15:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12229/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 15
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 16:54:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 25:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/common/skip.py
File tests/common/skip.py:
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/common/skip.py@28
PS25, Line 28: V
flake8: E501 line too long (93 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/common/skip.py@254
PS25, Line 254: class SkipIfApacheHive():
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/query_test/test_geospatial_functions.py
File tests/query_test/test_geospatial_functions.py:
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/query_test/test_geospatial_functions.py@21
PS25, Line 21: class TestGeospatialFuctions(ImpalaTestSuite):
flake8: E302 expected 2 blank lines, found 1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 25
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 10:32:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 25:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12324/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 25
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 10:53:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12188/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 14:24:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 13:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@49
PS13, Line 49: if (returnType == BytesWritable.class) {
: return ScalarType.createType(PrimitiveType.BINARY);
: }
A bool like "preferBinaryForBytesWritable_" could be used to decide this instead of having a separate class.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@66
PS13, Line 66: // We do not support varargs for Java UDFs, and neither does Hive.
This is no longer true (I think it was true when to original extract logic was written).
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
File fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java@39
PS13, Line 39: @Override
: public String toString() {
: return Double.toString(get());
: }
This seems to be a problem in other Impala*Writable too - can you create a Jira about this?
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 13
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jan 2023 16:37:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 23: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 23
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 14:00:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 18:
(10 comments)
Looks pretty clean. Mostly have a few questions, and we need to think about documentation.
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@31
PS18, Line 31: Known limitations:
This could all use some documentation. Let's at least file a ticket for it if you feel it's too early to publish documentation.
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@35
PS18, Line 35: - ST_MultiPoint, ST_LineStrin supports a maximum of 7
Should be ST_LineString?
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc
File be/src/exprs/hive-udf-call.cc:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc@195
PS18, Line 195: bool builtin_udf = fn_.hdfs_location.empty();
Why was this check necessary?
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt
File be/src/kudu/rpc/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt@110
PS18, Line 110: add_dependencies(protoc-gen-krpc thrift-deps ImpalaThrift)
Why was this needed? It looks a little weird to depend on Impala things in our borrowed kudu source.
http://gerrit.cloudera.org:8080/#/c/19425/18/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/18/common/function-registry/gen_geospatial_udf_wrappers.py@53
PS18, Line 53: UDF_PACKAGE = "org.apache.hadoop.hive.ql.udf.esri"
What library does this come from? Would be nice to document it to tie it back to how it's included (presumably pom.xml).
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@66
PS18, Line 66: fnArgsList,
nit: Formatting seems a little odd here.
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@156
PS18, Line 156: LOG.warn("Ignoring incompatible method: " + m + " during load of "
How about using slf4j variable substitution instead of concatenation?
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@171
PS18, Line 171: "No compatible signatures found for function:" + hiveFn_.getFunctionName());
nit: space after ":"
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
File fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java@32
PS18, Line 32: public List<ScalarFunction> extract() throws CatalogException {
nit: Presumably you don't need this override anymore, since the updated implementation will create a default HiveLegacyFunctionExtractor and pass it to the new signature.
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
File testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test:
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@1
PS18, Line 1: =====
Is this test set something we could generate? I guess at this point you've presumably enumerated all the function signatures.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 18
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jan 2023 22:51:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 26:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/common/skip.py
File tests/common/skip.py:
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/common/skip.py@255
PS25, Line 255:
Nit: -2 indentation, and the next line is a continuation line, so it should be indented +4 relative to this line.
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/custom_cluster/test_geospatial_library.py
File tests/custom_cluster/test_geospatial_library.py:
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/custom_cluster/test_geospatial_library.py@34
PS25, Line 34: @SkipIfApacheHive.feature_not_supported
Do we have to skip this test? If we're using Apache Hive, can't / shouldn't we verify that the functions are not present?
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 26
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 10:57:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 14:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG@18
PS13, Line 18: limitations
> Could mention that some of the limitations are that Impala doesn't support
Done
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG@34
PS13, Line 34: n
> Is there a reason for the different upper limits (6 and 7)?
That's the minimum setting where the tests still passing. It can be changed to a more reasonable limit, if there's a recommendation for it.
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/common/global-flags.cc@379
PS13, Line 379: "geospatial functions should be included as builtins. Possible values: "
> Could include in the description what the possible values are.
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@25
PS13, Line 25: # method. The value of n should be simultaneously
> A comment could describe what this file is used for and how it is used.
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@26
PS13, Line 26: e
> If we used triple quotes around the string we wouldn't have to put '\n\' at
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@28
PS13, Line 28:
> Is there a reason why the wording here is a bit different from the licence
It's based on gen_builtins_catalog.py's preamble string, changing to match the file's license
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@46
PS13, Line 46: or
> Nit: parameter number?
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@48
PS13, Line 48: // under
> We should document the structure of this list.
Implemented as a class
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@50
PS13, Line 50: // This is a generated file, DO NOT EDIT.
: // To add new functions, see the generator at
> optional: could be merged by just passing "ST_ConvexHull" and assuming that
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@52
PS13, Line 52: // common/function-registry/gen_geospatial_bu
> optional: return type could be omitted as all UDFs return BINARY
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@54
PS13, Line 54:
> optional: while impala_functions.py uses lists, using dictionary here could
Reimplemented as a class
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@75
PS13, Line 75:
:
: class Wrap
> optional: maybe using an array with the wanted arg counts would be clearer?
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@102
PS13, Line 102:
> Maybe 'num' would be better here and for 'generate_argument()'.
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@147
PS13, Line 147: Wrapper("%s.ST_LineString" % UDF_PACKAGE, DOUBLE_TYPE, range(2, 15, 2), True),
> nit: while this style make sense, it is not how we generally break lines at
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@167
PS13, Line 167:
> The file we generate is not very readable and also contains whitespace at t
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@58
PS13, Line 58: initBuiltins
> optional: I think that this function became a bit too long and could be bro
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@121
PS13, Line 121: createRelationalGenericUDFs(genericUDFs);
> This looks less than the values in the generator.
Varargs UDFs are now processed with function extraction, same as the legacy UDFs.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@155
PS13, Line 155:
> Nit: full stop instead of comma.
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@198
PS13, Line 198: function.setHasVarArgs(false);
> nit:missing blank line
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@207
PS13, Line 207: tSimpleName().toLowe
> If we multiply it with config.increment (on L210) then this is not really t
Removed
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@208
PS13, Line 208: }
> Why do we need this limit?
Removed
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@248
PS13, Line 248:
> Wouldn't 'wkIdWrap' be more informative?
This method is nearly identical to the original method in ST_SetSRID, only the WKID writing and logging part is changed. I would keep it as it is.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@261
PS13, Line 261:
> The error message doesn't seem to be correct - we get here if 'geomref' is
Replaced with the original call
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@34
PS13, Line 34: return ScalarType.createType(PrimitiveType.BINARY);
> Why don't we override resolveArgumentType() instead and let the parent clas
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@39
PS13, Line 39: }
> Is there a reason why we treat BytesWritable as a special case instead of m
My goal here was to keep the original mapping as it is for compatibility and make a separate implementation for the geospatial functions. If we change the original mapping, I think, it may break UDFs in the wild.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@49
PS13, Line 49:
:
:
> A bool like "preferBinaryForBytesWritable_" could be used to decide this in
I would say abstracting it with a class is much cleaner than a condition in the original implementation.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@66
PS13, Line 66: fnArgsList,
> This is no longer true (I think it was true when to original extract logic
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@81
PS13, Line 81:
> Could it be ScalarType? Do we allow complex-typed parameters?
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@90
PS13, Line 90: ption {
> This function could and resolveArgumentType() do the same thing, only the e
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
File fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java@39
PS13, Line 39: @Override
: public String toString() {
: return Double.toString(get());
: }
> This seems to be a problem in other Impala*Writable too - can you create a
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/service/BackendConfig.java@32
PS13, Line 32:
> nit: move to the impala. imports
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py@49
PS13, Line 49:
> nit: +2 indentation
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py@52
PS13, Line 52:
:
> could be simpler with
Done
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 18:08:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 15:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/19425/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/14//COMMIT_MSG@19
PS14, Line 19: only partial generics suppor
> Optional: we could say "and only partial ..."
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/util/backend-gflag-util.cc
File be/src/util/backend-gflag-util.cc:
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/util/backend-gflag-util.cc@341
PS13, Line 341: o_st
> The values of the possible strings could be extracted to constants somewher
Done
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@54
PS13, Line 54: UDF_PACKAGE
> About throws: in the new version always UDFArgumentException is thrown, whi
Restored to the original approach
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@20
PS14, Line 20:
> Thanks for the descriprion. Now it is formatted to have 60 character lines
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@55
PS14, Line 55: DOUBLE_TYPE = "org.apache.hadoop.hive.serde2.io.DoubleWritable"
> These constants would be more logically placed in the Wrapper class.
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@58
PS14, Line 58:
> Because of the newline at the end of METHOD_FORMAT there's an empty line in
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS14, Line 78:
> Wouldn't a boolean default argument make more sense?
To make throws clauses more flexible, now this parameter is used to pass the class name, and renamed to "throws", and the default value is None, showing that it throws nothing.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS14, Line 78:
> Why isn't it False?
I answered it in Csaba's comment
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@79
PS14, Line 79: meter_type
> We could call it self.original_class.
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@98
PS14, Line 98:
> Should be ", ".
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@104
PS14, Line 104:
> Should be ", ".
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/14/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@222
PS14, Line 222: /*
: TODO: IMPALA-11854: A workaround must be applied for ST_SetSRID UDF because the
: GeometryUtils.setWKID method assumes that the incoming geomref's buffer can
: be modified through the array returned by ImpalaBytesWritable.getBytes.
: */
> Can you mention IMPALA-11854 here?
Done
http://gerrit.cloudera.org:8080/#/c/19425/14/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/tests/query_test/test_udfs.py@347
PS14, Line 347: def test_udf_errors(self, vector, unique_database):
: # Only run with codegen disabled to force interpretation pa
> I would prefer to create a new class for this. test_udfs.py is a bit strang
Done
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 15
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 16:34:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12240/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 18
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 14:26:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 28:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9031/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 28
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 15:10:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#25). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
The ESRI geospatial implementation for Hive currently only
available in Hive 4, but CDP Hive backported it to Hive 3,
therefore for Apache Hive this feature is disabled
regardless of the "geospatial_library" flag.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
A fe/src/compat-apache-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
A fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
M tests/common/environ.py
M tests/common/skip.py
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
34 files changed, 3,541 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/25
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 25
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 2:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@160
PS2, Line 160: Generate ScalarFunctions for relational UDFs with a return type of boolean and two binary or string arguments
line too long (111 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java
File fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java@26
PS2, Line 26: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2);}
line too long (143 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java@27
PS2, Line 27: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3);}
line too long (169 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java@28
PS2, Line 28: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4);}
line too long (195 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java@29
PS2, Line 29: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4, BytesWritable arg5) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4, arg5);}
line too long (221 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/STUnionWrapper.java@30
PS2, Line 30: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4, BytesWritable arg5, BytesWritable arg6) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4, arg5, arg6);}
line too long (247 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java
File fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java@25
PS2, Line 25: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2);}
line too long (143 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java@26
PS2, Line 26: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3);}
line too long (169 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java@27
PS2, Line 27: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4);}
line too long (195 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java@28
PS2, Line 28: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4, BytesWritable arg5) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4, arg5);}
line too long (221 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/catalog/geospatial/StConvexHullWrapper.java@29
PS2, Line 29: public BytesWritable evaluate(BytesWritable arg1, BytesWritable arg2, BytesWritable arg3, BytesWritable arg4, BytesWritable arg5, BytesWritable arg6) throws UDFArgumentLengthException { return super.evaluate(arg1, arg2, arg3, arg4, arg5, arg6);}
line too long (247 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java@93
PS2, Line 93: public List<ScalarFunction> extract(HiveLegacyFunctionExtractor extractor) throws CatalogException {
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@46
PS2, Line 46: public List<ScalarFunction> extract(HiveLegacyFunctionExtractor extractor)throws CatalogException;
line too long (100 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@73
PS2, Line 73: protected List<Type> resolveArgumentTypes(List<Class<?>> arguments) throws ImpalaException{
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@140
PS2, Line 140: public List<ScalarFunction> extract(HiveLegacyFunctionExtractor extractor) throws CatalogException {
line too long (102 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
File fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java:
http://gerrit.cloudera.org:8080/#/c/19425/2/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java@37
PS2, Line 37: public List<ScalarFunction> extract(HiveLegacyFunctionExtractor extractor) throws CatalogException {
line too long (104 > 90)
http://gerrit.cloudera.org:8080/#/c/19425/2/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/2/tests/custom_cluster/test_geospatial_udfs.py@23
PS2, Line 23: class TestGeospatialUdfs(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/2/tests/custom_cluster/test_geospatial_udfs.py@45
PS2, Line 45:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 14:26:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
WIP IMPALA-11745
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
M testdata/datasets/functional/functional_schema_template.sql
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_geospatial_udfs.py
M tests/query_test/test_udfs.py
31 files changed, 3,503 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/5
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 13:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG@18
PS13, Line 18: limitations
Could mention that some of the limitations are that Impala doesn't support varargs and generics.
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG@34
PS13, Line 34: 7
Is there a reason for the different upper limits (6 and 7)?
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/common/global-flags.cc@379
PS13, Line 379: "Specifies which implementation of geospatial functions should be included "
Could include in the description what the possible values are.
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/util/backend-gflag-util.cc
File be/src/util/backend-gflag-util.cc:
http://gerrit.cloudera.org:8080/#/c/19425/13/be/src/util/backend-gflag-util.cc@341
PS13, Line 341: NONE
The values of the possible strings could be extracted to constants somewhere and used here and in global-flags.cc.
Thrift now supports operator<< for enums, so we could also get the string values from there.
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@25
PS13, Line 25:
A comment could describe what this file is used for and how it is used.
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@26
PS13, Line 26: '
If we used triple quotes around the string we wouldn't have to put '\n\' at the end of each line.
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@28
PS13, Line 28: // Licensed under the Apache License, Version 2.0 (the "License");\n\
Is there a reason why the wording here is a bit different from the licence header in this source file?
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@46
PS13, Line 46: size
Nit: parameter number?
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@48
PS13, Line 48: WRAPPERS
We should document the structure of this list.
We could also use dictionaries or a class for the elements.
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@102
PS13, Line 102: order
Maybe 'num' would be better here and for 'generate_argument()'.
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@167
PS13, Line 167: def generate_file(config):
The file we generate is not very readable and also contains whitespace at the end of some lines.
We could make it a bit more readable by adding newlines in and between method definitions etc.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@155
PS13, Line 155: ,
Nit: full stop instead of comma.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@207
PS13, Line 207: minNumberOfArguments
If we multiply it with config.increment (on L210) then this is not really the min number of arguments (and the same for the max number). We should come up with a better name and also add a doc comment about this in WrapperConfig.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@208
PS13, Line 208: .limit(config.maxNumberOfArguments)
Why do we need this limit?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@248
PS13, Line 248: wkwrap
Wouldn't 'wkIdWrap' be more informative?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@261
PS13, Line 261: LOG.error("Invalid arguments - one or more arguments are null.");
The error message doesn't seem to be correct - we get here if 'geomref' is null or empty, the other parameter ('wkwrap') is not checked in the corresponding if condition.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@34
PS13, Line 34: protected List<Type> resolveArgumentTypes(List<Class<?>> arguments)
Why don't we override resolveArgumentType() instead and let the parent class handle the loop?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@39
PS13, Line 39: if (argument == BytesWritable.class) {
Is there a reason why we treat BytesWritable as a special case instead of modifying JavaUdfDataType.java to map it to TPrimitiveType.BINARY? Does it need to map to STRING in other cases?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@81
PS13, Line 81: Type
Could it be ScalarType? Do we allow complex-typed parameters?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@90
PS13, Line 90: resolveReturnType
This function could and resolveArgumentType() do the same thing, only the error message is different. To reduce code duplication, they could both delegate to a function that has one more parameter for whether it is an argument or a return type.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 13
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jan 2023 11:03:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 23:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12310/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 23
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 14:12:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12287/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 20
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Feb 2023 14:50:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 24:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9022/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 24
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 15:18:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
WIP IMPALA-11745
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
M testdata/datasets/functional/functional_schema_template.sql
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_geospatial_udfs.py
M tests/query_test/test_udfs.py
31 files changed, 3,504 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/6
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 6
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 16: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 16
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 16:44:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#18). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/kudu/rpc/CMakeLists.txt
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
31 files changed, 3,520 insertions(+), 152 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/18
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 18
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19425/14/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/14/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@222
PS14, Line 222: /*
: A workaround must be applied for ST_SetSRID UDF because the GeometryUtils.setWKID
: method assumes that the incoming geomref's buffer can be modified through the array
: returned by ImpalaBytesWritable.getBytes.
: */
Can you mention IMPALA-11854 here?
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 10:46:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#20). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
30 files changed, 3,496 insertions(+), 154 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/20
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 20
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 20: Code-Review+1
(6 comments)
Looks pretty good. I still have a few things I think would be worth cleaning up.
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@31
PS18, Line 31: Known limitations:
> https://issues.apache.org/jira/browse/IMPALA-11889 created
Done
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc
File be/src/exprs/hive-udf-call.cc:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc@195
PS18, Line 195: bool builtin_udf = fn_.hdfs_location.empty();
> Before this patch Impala has assumed that all Hive UDFs come for external j
I'd feel more comfortable with a code comment describing the expectations here.
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt
File be/src/kudu/rpc/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt@110
PS18, Line 110: add_dependencies(protoc-gen-krpc thrift-deps)
> Removed.
Oh, it was because the executable links global-flags.cc. And I guess types.h can't be included without the library.
http://gerrit.cloudera.org:8080/#/c/19425/20/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/19425/20/bin/start-impala-cluster.py@431
PS20, Line 431: args = "{args} -geospatial_library {geospatial_library}".format(
Why did this change?
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
File fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java@32
PS18, Line 32: return extract(new HiveLegacyFunctionExtractor());
> I don't really understand what override removing means in this context, I r
I meant you could remove this version of the signature from TestHiveJavaFunction. The original would work just as well. I think explicitly re-implementing https://gerrit.cloudera.org/c/19425/20/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java#89 is more confusing.
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
File testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test:
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@1
PS18, Line 1: =====
> These tests are coming from https://github.com/Esri/spatial-framework-for-h
What's here seems fine.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 20
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Feb 2023 17:39:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 23:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@46
PS22, Line 46: }
> Nit: -2 indentation.
Done
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
File fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java:
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java@151
PS22, Line 151:
> Can we generate the expected name here the same way the actual name is gene
Done
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 23
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 13:52:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 14:
(5 comments)
looks good to me in general, just a few small comments
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@54
PS13, Line 54:
> Reimplemented as a class
About throws: in the new version always UDFArgumentException is thrown, while in the old version StPolygonWrapper threw UDFArgumentLengthException. Is this change intentional?
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS14, Line 78: can_throw=""
Wouldn't a boolean default argument make more sense?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@39
PS13, Line 39: }
> My goal here was to keep the original mapping as it is for compatibility an
There is a Jira about this which explains the issues: IMPALA-11340
It could be mentioned in a comment.
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java@49
PS13, Line 49:
:
:
> I would say abstracting it with a class is much cleaner than a condition in
ack
http://gerrit.cloudera.org:8080/#/c/19425/14/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/tests/query_test/test_udfs.py@347
PS14, Line 347: def test_esri_geospatial_functions(self, vector):
: self.run_test_case('QueryTest/udf-esri-geospatial', vector)
I would prefer to create a new class for this. test_udfs.py is a bit strange place for the tests, as from the external point of view these are builtins, not UDFs. + I hope that many other geospatial test will follow so collecting them in geospatial specific test suite makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 10:38:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 4:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@47
PS4, Line 47: e
flake8: E501 line too long (169 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@48
PS4, Line 48: e
flake8: E501 line too long (234 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@49
PS4, Line 49: e
flake8: E501 line too long (234 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@50
PS4, Line 50: o
flake8: E501 line too long (234 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@51
PS4, Line 51: o
flake8: E501 line too long (159 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@54
PS4, Line 54: def generate_parameter(parameter_type, order):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@55
PS4, Line 55: )
flake8: E501 line too long (91 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@57
PS4, Line 57: def generate_argument(order):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@60
PS4, Line 60: def generate_argument_list(order):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@66
PS4, Line 66: def generate_parameter_list(parameter_type, order):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@72
PS4, Line 72: def generate_method(return_type, parameter_type, exception_type, order):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@76
PS4, Line 76: u
flake8: E501 line too long (300 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@76
PS4, Line 76:
flake8: E202 whitespace before ')'
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS4, Line 78: def generate_methods(return_type, parameter_type, exception_type, min_number_of_params, max_number_of_params, increment):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS4, Line 78: x
flake8: E501 line too long (121 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@81
PS4, Line 81:
flake8: E203 whitespace before ','
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@82
PS4, Line 82:
flake8: E202 whitespace before ')'
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@86
PS4, Line 86: def generate_wrapper_class(config):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@87
PS4, Line 87: a
flake8: E501 line too long (218 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@89
PS4, Line 89: def generate_file(config):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@90
PS4, Line 90: g
flake8: E501 line too long (152 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@92
PS4, Line 92: if not os.path.exists(FE_PATH):
flake8: E305 expected 2 blank lines after class or function definition, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/common/function-registry/gen_geospatial_udf_wrappers.py@100
PS4, Line 100:
flake8: W292 no newline at end of file
http://gerrit.cloudera.org:8080/#/c/19425/4/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/4/tests/custom_cluster/test_geospatial_udfs.py@23
PS4, Line 23: class TestGeospatialUdfs(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/4/tests/custom_cluster/test_geospatial_udfs.py@45
PS4, Line 45:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 14:05:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library as builtin UDFs.
Plain Hive UDFs are imported without changes, but the generic and varargs
functions are handled differently; generic functions are added with all of the
combinations of their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple function. The varargs
function wrappers are generated at build time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are required because of
the limitations in Impala's UDFExecuter which could be further improved; in
this case, the additional wrapping/mapping steps could be removed.
A new backend flag was added to turn this feature on/off as "geospatial_library".
The default value is "NONE" which means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7 pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py and
HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop/tree/master/hive/test
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
A tests/custom_cluster/test_geospatial_udfs.py
27 files changed, 3,505 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/10
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 10
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
A tests/custom_cluster/test_geospatial_udfs.py
28 files changed, 3,527 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/13
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 13
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19425/12/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/12/tests/custom_cluster/test_geospatial_udfs.py@26
PS12, Line 26: class TestGeospatialUdfs(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 12
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jan 2023 21:01:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#28). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
The ESRI geospatial implementation for Hive currently only
available in Hive 4, but CDP Hive backported it to Hive 3,
therefore for Apache Hive this feature is disabled
regardless of the "geospatial_library" flag.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
A fe/src/compat-apache-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
A fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
M tests/common/environ.py
M tests/common/skip.py
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
34 files changed, 3,544 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/28
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 28
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 24:
Glimpsed into the failed build - most test passed, the only thing that failed was https://jenkins.impala.io/job/all-build-options-ub1604/9262
My guess is that the failure happened when trying to build Impala with apache Hive (instead of Cloudera version).
This should be reproduceable after setting env var USE_APACHE_HIVE and rebuilding Impala.
See https://github.com/apache/impala/commit/1306c58f29dee4f8f4c0ab18327bc19557a9156e for more info
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 24
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Sat, 04 Feb 2023 09:25:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 16:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12230/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 16
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 16:57:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#16). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/kudu/rpc/CMakeLists.txt
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
30 files changed, 3,506 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/16
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 16
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 17: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19425/17/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/19425/17/be/src/common/global-flags.cc@385
PS17, Line 385: NONE
We could also use the to_string() call here, or even better put both options into constants and use that.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@20
PS14, Line 20:
> Done
Nit: it's still 80, not 90.
http://gerrit.cloudera.org:8080/#/c/19425/17/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/17/common/function-registry/gen_geospatial_udf_wrappers.py@80
PS17, Line 80: can_throw
This could also be renamed to 'self.throws'.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 17
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 12:56:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#23). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
31 files changed, 3,490 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/23
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 23
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 22:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19425/20/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:
http://gerrit.cloudera.org:8080/#/c/19425/20/bin/start-impala-cluster.py@431
PS20, Line 431: args = "{args} -geospatial_library={geospatial_library}".format(
> Why did this change?
Restored to the {}={} pattern, I got confused about which argument passing format is used here
http://gerrit.cloudera.org:8080/#/c/19425/21/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/21/fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java@88
PS21, Line 88: public List<ScalarFunction> extract(HiveLegacyFunctionExtractor extractor)
> nit: not needed because the interface provides a default.
Done
http://gerrit.cloudera.org:8080/#/c/19425/21/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/21/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@44
PS21, Line 44: public default List<ScalarFunction> extract() throws CatalogException {
> Oh, I think I missed that they were all implementing an interface rather th
Done
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 22
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 08:24:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 26: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/custom_cluster/test_geospatial_library.py
File tests/custom_cluster/test_geospatial_library.py:
http://gerrit.cloudera.org:8080/#/c/19425/25/tests/custom_cluster/test_geospatial_library.py@34
PS25, Line 34: @SkipIfApacheHive.feature_not_supported
> Do we have to skip this test? If we're using Apache Hive, can't / shouldn't
AFAIK the current state of Impala with Apache Hive is that the dataload is working, but probably a lot of tests are failing:
https://gerrit.cloudera.org/#/c/18028/
I don't think that we need to verify that the geo functions do not exist in Apache Hive - we can recheck all SkipIfApacheHive on a Hive version bump.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 26
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 11:54:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12206/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 12
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jan 2023 21:20:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
The ESRI geospatial implementation for Hive currently only
available in Hive 4, but CDP Hive backported it to Hive 3,
therefore for Apache Hive this feature is disabled
regardless of the "geospatial_library" flag.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Reviewed-on: http://gerrit.cloudera.org:8080/19425
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
A fe/src/compat-apache-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
A fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
M tests/common/environ.py
M tests/common/skip.py
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
34 files changed, 3,544 insertions(+), 158 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 29
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 28: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 28
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 20:18:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 24: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 24
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 00:39:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#26). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
The ESRI geospatial implementation for Hive currently only
available in Hive 4, but CDP Hive backported it to Hive 3,
therefore for Apache Hive this feature is disabled
regardless of the "geospatial_library" flag.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
A fe/src/compat-apache-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
A fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
M tests/common/environ.py
M tests/common/skip.py
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
34 files changed, 3,544 insertions(+), 158 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/26
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 26
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19425/5/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/5/tests/custom_cluster/test_geospatial_udfs.py@23
PS5, Line 23: class TestGeospatialUdfs(CustomClusterTestSuite):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/5/tests/custom_cluster/test_geospatial_udfs.py@45
PS5, Line 45:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 14:36:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 17:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12232/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 17
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 18:03:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19425/15/tests/query_test/test_geospatial_functions.py
File tests/query_test/test_geospatial_functions.py:
http://gerrit.cloudera.org:8080/#/c/19425/15/tests/query_test/test_geospatial_functions.py@20
PS15, Line 20: class TestGeospatialFuctions(ImpalaTestSuite):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/19425/15/tests/query_test/test_geospatial_functions.py@28
PS15, Line 28:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 15
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 16:35:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 18:
(3 comments)
Answering some of the questions as they are related to code/decisions from my original prototype,
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@31
PS18, Line 31: Known limitations:
> This could all use some documentation. Let's at least file a ticket for it
+1 for the doc Jira
The original Esri framework for Hadoop has documentation for all UDFs: https://github.com/Esri/spatial-framework-for-hadoop/wiki/UDF-Documentation
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc
File be/src/exprs/hive-udf-call.cc:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/exprs/hive-udf-call.cc@195
PS18, Line 195: bool builtin_udf = fn_.hdfs_location.empty();
> Why was this check necessary?
Before this patch Impala has assumed that all Hive UDFs come for external jars. These jars are fetched from HDFS and copied local when needed. This is handled by LibCache. The ESRI geospatial UDFs are imported via Hive when building Impala and are assumed to be always available, so there is no need to get LibCache involved.
This is handled a bit differently for builtin native function: https://github.com/apache/impala/blob/master/be/src/exprs/scalar-fn-call.cc#L116
Builtin native functions are also differentiated in enum TFunctionBinaryType (NATIVE vs BUILTIN), while for Hive UDFs JAVA is in both cases. Adding JAVA_BUILTIN to the enum would have only extra noise without benefits as most of the code doesn't need specialized handling for builtin Java UDFs.
Also LibCache helps in the handling of native builtins (returning functions pointers based on symbol name), but this is not needed for Hive UDFs.
http://gerrit.cloudera.org:8080/#/c/19425/18/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/18/common/function-registry/gen_geospatial_udf_wrappers.py@53
PS18, Line 53: UDF_PACKAGE = "org.apache.hadoop.hive.ql.udf.esri"
> What library does this come from? Would be nice to document it to tie it ba
Originally this comes from
https://github.com/Esri/gis-tools-for-hadoop
which relies on
https://github.com/Esri/geometry-api-java
The content of gis-tools-for-hadoop was copied to Hive source code in HIVE-26223 https://github.com/apache/hive/commit/6bfb6f6a3676ac692d92a08b88f439794f20b488
geometry-api-java is pulled through Maven
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 18
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Feb 2023 08:14:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#22). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineString supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
31 files changed, 3,489 insertions(+), 157 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/22
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 22
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
A tests/custom_cluster/test_geospatial_udfs.py
28 files changed, 3,527 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/12
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 12
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12207/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 13
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jan 2023 21:23:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#15). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/kudu/rpc/CMakeLists.txt
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
30 files changed, 3,505 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/15
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 15
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 20:
(8 comments)
Thank you, Michael!
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@31
PS18, Line 31: Known limitations:
> +1 for the doc Jira
https://issues.apache.org/jira/browse/IMPALA-11889 created
http://gerrit.cloudera.org:8080/#/c/19425/18//COMMIT_MSG@35
PS18, Line 35: - ST_MultiPoint, ST_LineString supports a maximum of 7
> Should be ST_LineString?
Done
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt
File be/src/kudu/rpc/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/19425/18/be/src/kudu/rpc/CMakeLists.txt@110
PS18, Line 110: add_dependencies(protoc-gen-krpc thrift-deps)
> Why was this needed? It looks a little weird to depend on Impala things in
Removed.
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java@66
PS18, Line 66: fn.setBinaryType(TFunctionBinaryType.JAVA);
> nit: Formatting seems a little odd here.
Done
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@156
PS18, Line 156: } else {
> How about using slf4j variable substitution instead of concatenation?
Done
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java@171
PS18, Line 171: if (result.isEmpty()) {
> nit: space after ":"
Done
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
File fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java:
http://gerrit.cloudera.org:8080/#/c/19425/18/fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java@32
PS18, Line 32: return extract(new HiveLegacyFunctionExtractor());
> nit: Presumably you don't need this override anymore, since the updated imp
I don't really understand what override removing means in this context, I replaced it with a call to the overload with HiveLegacyFunctionExtractor as an argument.
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
File testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test:
http://gerrit.cloudera.org:8080/#/c/19425/18/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@1
PS18, Line 1: =====
> Is this test set something we could generate? I guess at this point you've
These tests are coming from https://github.com/Esri/spatial-framework-for-hadoop/tree/master/hive/test, and they got transformed to Impala's test format by my limited-knowledge python script. The main concern with generation is that the results do not have type information in the source repository, and I had to apply multiple regex replaces manually to make all of the tests work in Impala. We talked with Csaba that we'll use these tests for the initial phase of geospatial support, and later on, we should add a more broad test set.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 20
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Feb 2023 14:34:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 23: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 23
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 14:47:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 18: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 18
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 14:05:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 26:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12326/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 26
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 11:08:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 13:
(11 comments)
Looked through most of the code (with the exception of the extraction handling / BINARY support and the tests), looks great!
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@50
PS13, Line 50: "StConvexHullWrapper",
: "org.apache.hadoop.hive.ql.udf.esri.ST_ConvexHull",
optional: could be merged by just passing "ST_ConvexHull" and assuming that "org.apache.hadoop.hive.ql.udf.esri" is the prefix + renaming the wrapper to ST_ConvexHull_Wrapper
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@52
PS13, Line 52: "org.apache.hadoop.io.BytesWritable",
optional: return type could be omitted as all UDFs return BINARY
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@54
PS13, Line 54: "",
optional: while impala_functions.py uses lists, using dictionary here could be clearer and also allow unused elements to be unspecified, 'throws'= 'exception' when the function can actually throw
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@75
PS13, Line 75: 2,
: 14,
: 2,
optional: maybe using an array with the wanted arg counts would be clearer? e.g. range(2,15,2) or [2, 4, 6, 8, 10, 12, 14]
http://gerrit.cloudera.org:8080/#/c/19425/13/common/function-registry/gen_geospatial_udf_wrappers.py@147
PS13, Line 147: increment,
nit: while this style make sense, it is not how we generally break lines at function signatures in Impala
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@58
PS13, Line 58: initBuiltins
optional: I think that this function became a bit too long and could be broken up based on the way the function is added.
e.g. addLegacyUDFs(Db db), addWrappedVarargUDFs(Db db) ...
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@121
PS13, Line 121: WrapperConfig polygonConfig = new WrapperConfig(Type.DOUBLE, Type.BINARY, 3, 6, 2);
This looks less than the values in the generator.
Maybe the configs could be also generated, e.g. through static members read with reflection?
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@198
PS13, Line 198: /*
nit:missing blank line
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/service/BackendConfig.java
File fe/src/main/java/org/apache/impala/service/BackendConfig.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/service/BackendConfig.java@32
PS13, Line 32: import org.apache.impala.thrift.TGeospatialLibrary;
nit: move to the impala. imports
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py
File tests/custom_cluster/test_geospatial_udfs.py:
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py@49
PS13, Line 49: start_args='--geospatial_library=NONE')
nit: +2 indentation
http://gerrit.cloudera.org:8080/#/c/19425/13/tests/custom_cluster/test_geospatial_udfs.py@52
PS13, Line 52: self.execute_query("use _impala_builtins")
: result = self.execute_query("show functions")
could be simpler with
"show functions in _impala_builtins"
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 13
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jan 2023 11:49:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 14:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/13//COMMIT_MSG@34
PS13, Line 34: n
> That's the minimum setting where the tests still passing. It can be changed
I see. We can leave it that way or choose a common number, either is fine for me.
http://gerrit.cloudera.org:8080/#/c/19425/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19425/14//COMMIT_MSG@19
PS14, Line 19: and partial generics support
Optional: we could say "and only partial ..."
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py
File common/function-registry/gen_geospatial_udf_wrappers.py:
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@20
PS14, Line 20:
Thanks for the descriprion. Now it is formatted to have 60 character lines but we allow 90.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@55
PS14, Line 55: METHOD_FORMAT = """ public {return_type} evaluate({parameter_list}) {exception_clause}{{
These constants would be more logically placed in the Wrapper class.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@58
PS14, Line 58: """
Because of the newline at the end of METHOD_FORMAT there's an empty line in the class after the last method. We could remove this newline here and use "\n\n" as the joining string instead.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@78
PS14, Line 78: ""
Why isn't it False?
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@79
PS14, Line 79: base_class
We could call it self.original_class.
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@98
PS14, Line 98: ,
Should be ", ".
http://gerrit.cloudera.org:8080/#/c/19425/14/common/function-registry/gen_geospatial_udf_wrappers.py@104
PS14, Line 104: ,
Should be ", ".
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
File fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java:
http://gerrit.cloudera.org:8080/#/c/19425/13/fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java@261
PS13, Line 261:
> Replaced with the original call
What does this method print?
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Jan 2023 13:12:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12226/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jan 2023 18:25:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and and partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
A tests/custom_cluster/test_geospatial_library.py
M tests/query_test/test_udfs.py
29 files changed, 3,479 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/14
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 14
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 28:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12328/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 28
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Feb 2023 15:12:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
WIP IMPALA-11745
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/util/backend-gflag-util.cc
M common/function-registry/CMakeLists.txt
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaStringWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
M testdata/datasets/functional/functional_schema_template.sql
A testdata/workloads/functional-query/queries/QueryTest/udf-esri-geospatial.test
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_geospatial_udfs.py
M tests/query_test/test_udfs.py
31 files changed, 3,422 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/4
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12190/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 6
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 15:00:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-11745
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: WIP IMPALA-11745
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/12178/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 14:47:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 22:
(2 comments)
Looks good, just a few things.
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
File fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java:
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java@46
PS22, Line 46:
Nit: -2 indentation.
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java
File fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java:
http://gerrit.cloudera.org:8080/#/c/19425/22/fe/src/test/java/org/apache/impala/hive/executor/HiveLegacyJavaFunctionTest.java@151
PS22, Line 151: dummyudffn
Can we generate the expected name here the same way the actual name is generated?
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 22
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 13:12:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 24: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 24
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 15:18:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 24: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9022/
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 24
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 20:27:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
Patch Set 22:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12305/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 22
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Feb 2023 08:44:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has uploaded a new patch set (#17). ( http://gerrit.cloudera.org:8080/19425 )
Change subject: IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
......................................................................
IMPALA-11745: Add Hive's ESRI geospatial functions as builtins
This change adds geospatial functions from Hive's ESRI library
as builtin UDFs. Plain Hive UDFs are imported without changes,
but the generic and varargs functions are handled differently;
generic functions are added with all of the combinations of
their parameters (cartesian product of the parameters), and
varargs functions are unfolded as an nth parameter simple
function. The varargs function wrappers are generated at build
time and they can be configured in
gen_geospatial_udf_wrappers.py. These additional steps are
required because of the limitations in Impala's UDF Executor
(lack of varargs support and only partial generics support)
which could be further improved; in this case, the additional
wrapping/mapping steps could be removed.
Changes regarding function handling/creating are sourced from
https://gerrit.cloudera.org/c/19177
A new backend flag was added to turn this feature on/off
as "geospatial_library". The default value is "NONE" which
means no geospatial function gets registered
as builtin, "HIVE_ESRI" value enables this implementation.
Known limitations:
- ST_MultiLineString, ST_MultiPolygon only works
with the WKT overload
- ST_Polygon supports a maximum of 6 pairs of coordinates
- ST_MultiPoint, ST_LineStrin supports a maximum of 7
pairs of coordinates
- ST_ConvexHull, ST_Union supports a maximum of 6 geoms
These limits can be increased in gen_geospatial_udf_wrappers.py
and HiveEsriGeospatialBuiltins.java
Tests:
- test_geospatial_udfs.py added based on
https://github.com/Esri/spatial-framework-for-hadoop
Co-Authored-by: Csaba Ringhofer <cs...@cloudera.com>
Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
---
M be/src/common/global-flags.cc
M be/src/exprs/hive-udf-call.cc
M be/src/kudu/rpc/CMakeLists.txt
M be/src/util/backend-gflag-util.cc
M bin/start-impala-cluster.py
M common/function-registry/CMakeLists.txt
M common/function-registry/gen_builtins_catalog.py
A common/function-registry/gen_geospatial_udf_wrappers.py
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/HiveEsriGeospatialBuiltins.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
A fe/src/main/java/org/apache/impala/hive/executor/BinaryToBinaryHiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveGenericJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactory.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunctionFactoryImpl.java
A fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyFunctionExtractor.java
M fe/src/main/java/org/apache/impala/hive/executor/HiveLegacyJavaFunction.java
M fe/src/main/java/org/apache/impala/hive/executor/ImpalaDoubleWritable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/hive/executor/TestHiveJavaFunctionFactory.java
M java/CMakeLists.txt
M java/shaded-deps/hive-exec/pom.xml
M testdata/datasets/README
A testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test
A tests/custom_cluster/test_geospatial_library.py
A tests/query_test/test_geospatial_functions.py
31 files changed, 3,520 insertions(+), 152 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/19425/17
--
To view, visit http://gerrit.cloudera.org:8080/19425
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0ca02a70b4ba244778c9db6d14df4423072b225
Gerrit-Change-Number: 19425
Gerrit-PatchSet: 17
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>