Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24536 )
Change subject: POC IMPALA-15162: Add GEOMETRY column support for Iceberg and Parquet ...................................................................... Patch Set 8: (6 comments) thanks for the comments! fixed some of them this patch is still in POC state and wouldn't polish it too much until adding GEOMETRY type is merged https://gerrit.cloudera.org/#/c/24534/ I also hope that I can avoid the Iceberg patching if https://github.com/apache/iceberg/pull/17112 gets merged http://gerrit.cloudera.org:8080/#/c/24536/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24536/6//COMMIT_MSG@19 PS6, Line 19: KB_EXPERIMENTAL mode. > We dont check this yet in AlterTableAddColsStmt. thx, added check + test for that I am thinking about moving that part to parent patch https://gerrit.cloudera.org/#/c/24534/ http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java: http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java@136 PS6, Line 136: Iceberg tables, only default values are supported as colum > We are only validating the format-version, I think we should use similar lo I wouldn't check library here - for DDLs it doesn't really matter, its main goal is to switch how geospatial functions behave (return type and binary serialization). Also, at some point I would remove HIVE_ESRI in the future, it was never really supported. http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/analysis/TableDef.java@473 PS6, Line 473: .getColName().toLowerCas > nit: Might throw an uncaught exception if a non-numeric format-version stri Done http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java File fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java: http://gerrit.cloudera.org:8080/#/c/24536/6/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@382 PS6, Line 382: case STRING: > Should we also add a GEOGRAPHY case in here, since its present in HiveSchem Good question, it is enticing to add both at the same time, but currently we have no functions for geography. It may make sense to add minimal support for geography, e.g. WKT and WKB functions as those are the same as for geometry. This is mainly a question for prev patch https://gerrit.cloudera.org/#/c/24534/ http://gerrit.cloudera.org:8080/#/c/24536/6/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/24536/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4261 PS6, Line 4261: AnalyzesOk(String.format( > stale todo. Done http://gerrit.cloudera.org:8080/#/c/24536/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test: PS6: > Maybe we should add some negative test cases, like rejecting GEOMETRY on no Done -- To view, visit http://gerrit.cloudera.org:8080/24536 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32d8c12a6b71708646fbfc06a6dd8f7aaf4f5e6b Gerrit-Change-Number: 24536 Gerrit-PatchSet: 8 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-Comment-Date: Wed, 05 Aug 2026 12:11:13 +0000 Gerrit-HasComments: Yes
