Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24534 )
Change subject: IMPALA-15161: Add GEOMETRY type + make WKB_EXPERIMENTAL default ...................................................................... Patch Set 31: (12 comments) http://gerrit.cloudera.org:8080/#/c/24534/31//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24534/31//COMMIT_MSG@56 PS31, Line 56: bacause typo http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java File fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java: http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java@61 PS31, Line 61: LOG logger not used anywhere http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java@62 PS31, Line 62: nit: No indentation. http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java@82 PS31, Line 82: if(addNatives) nit: No gap between if and parentheses. http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/compat-hive-3/java/org/apache/impala/compat/GeospatialBuiltins.java@107 PS31, Line 107: ,new nit: no gap between comma and new. http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java File fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java: http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/compat-hive-3/java/org/apache/impala/compat/HiveEsriGeospatialBuiltins.java@112 PS13, Line 112: : : : : : : : > These functions state this pretty clearly in its description IMO: "construc Done http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@119 PS31, Line 119: nit: No indentation. http://gerrit.cloudera.org:8080/#/c/24534/31/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1748 PS31, Line 1748: nit: extra space. http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/main/java/org/apache/impala/catalog/Type.java@234 PS13, Line 234: public boolean isWildcardChar() { return false; } : : public boolean isStringType() { : > Good point - looked around how this function is used, and I think that it i Done http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4320 PS13, Line 4320: AnalyzesOk(String.format( > Added a describe test in geometry-type.test Done http://gerrit.cloudera.org:8080/#/c/24534/31/java/hive-geospatial-functions/src/main/java/org/apache/impala/hive/geospatial/esri/README.md File java/hive-geospatial-functions/src/main/java/org/apache/impala/hive/geospatial/esri/README.md: http://gerrit.cloudera.org:8080/#/c/24534/31/java/hive-geospatial-functions/src/main/java/org/apache/impala/hive/geospatial/esri/README.md@48 PS31, Line 48: `ST_Intersects`, `ST_Contains`, etc. : are registered only for `(BINARY, BINARY)` argument types; I think this is stale as (GEOMETRY, GEOMETRY) is now also registered in WKB mode. http://gerrit.cloudera.org:8080/#/c/24534/31/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/24534/31/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test@a2455 PS31, Line 2455: I might be missing something, but I dont understand why was this removed. This is making the test weaker. -- To view, visit http://gerrit.cloudera.org:8080/24534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ff29cb15ab45f3899bf453041e605a32f195c27 Gerrit-Change-Number: 24534 Gerrit-PatchSet: 31 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Mon, 21 Sep 2026 07:09:15 +0000 Gerrit-HasComments: Yes
