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 13: (11 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@15 PS13, Line 15: case.. two fullstops http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@21 PS13, Line 21: GEOMTRY typo http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@32 PS13, Line 32: cange typo: change http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@44 PS13, Line 44: analyical typo http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@56 PS13, Line 56: bacause typo 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: List<UDF> binaryInputConstructors = Arrays.asList( : new ST_GeomFromWKB(), new ST_GeomFromShape(), : new ST_PointFromWKB(), new ST_LineFromWKB(), : new ST_MLineFromWKB(), new ST_MPointFromWKB(), : new ST_MPolyFromWKB(), new ST_PolyFromWKB()); : : // Functions that serialize geometry to raw bytes (GEOMETRY -> BINARY in WKB mode). : List<UDF> serializers = Arrays.asList(new ST_AsBinary(), new ST_AsShape()); In WKB mode, ST_GeomFromShape / ST_AsShape are registered as BINARY <-> GEOMETRY but still run ESRI Shape UDFs. I think we could mention it in a comment somewhere. http://gerrit.cloudera.org:8080/#/c/24534/13/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/13/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1045 PS13, Line 1045: can' nit: can't http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1045 PS13, Line 1045: dont nit: don't 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 isStringType() { : return isScalarType(PrimitiveType.STRING) || isScalarType(PrimitiveType.VARCHAR) || : isScalarType(PrimitiveType.CHAR) || isScalarType(PrimitiveType.BINARY); : } Shouldn't GEOMETRY be included here too? In some callers like HdfsScanNode.java or ComputeStatsStmt.java, it may behave differently for GEOMETRY vs BINARY. 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("create view new_view as select cast(NULL as geometry) as g"); Can we test that CREATE VIEW stores binary in HMS while Impala keeps GEOMETRY type internally. http://gerrit.cloudera.org:8080/#/c/24534/13/testdata/workloads/functional-query/queries/QueryTest/geometry-type.test File testdata/workloads/functional-query/queries/QueryTest/geometry-type.test: http://gerrit.cloudera.org:8080/#/c/24534/13/testdata/workloads/functional-query/queries/QueryTest/geometry-type.test@37 PS13, Line 37: ==== Can we have a test case for DESCRIBE geom_view -- 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: 13 Gerrit-Owner: Csaba Ringhofer <[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: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Fri, 14 Aug 2026 05:54:32 +0000 Gerrit-HasComments: Yes
