Csaba Ringhofer 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 17: (10 comments) Thanks for the comments! Patches after PS14 rebased the change + fixed Arnab's comments. Haven't reacted to Peter's comments yet. 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 Done http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@21 PS13, Line 21: GEOMETR > typo Done http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@32 PS13, Line 32: chang > typo: change Done http://gerrit.cloudera.org:8080/#/c/24534/13//COMMIT_MSG@44 PS13, Line 44: analytica > typo Done 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 <-> GEO These functions state this pretty clearly in its description IMO: "construct ST_Geometry from Esri shape representation of geometry" / "return Esri shape representation of geometry". 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 Done http://gerrit.cloudera.org:8080/#/c/24534/13/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1045 PS13, Line 1045: don' > nit: don't Done 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. Good point - looked around how this function is used, and I think that it is a big mess. Created IMPALA-15279 to clean it up. For now I would not add it ,and BINARY is also questionable. 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 GEOMET Added a describe test in geometry-type.test 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: # Test inline views and union all. > Can we have a test case for DESCRIBE geom_view Done -- 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: 17 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: Jason Fehr <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Sat, 22 Aug 2026 19:41:15 +0000 Gerrit-HasComments: Yes
