Arnab Karmakar 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 6: (6 comments) Thanks for working on this! 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: WKB_EXPERIMENTAL mode We dont check this yet in AlterTableAddColsStmt. 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: iceTbl.getFormatVersion() < IcebergTable.ICEBERG_FORMAT_V3 We are only validating the format-version, I think we should use similar logic from TableDef.java. ``` if (!(isIcebergTable() && BackendConfig.INSTANCE.getGeospatialLibrary() == TGeospatialLibrary.WKB_EXPERIMENTAL)) ``` 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: Integer.parseInt(fmtVer) nit: Might throw an uncaught exception if a non-numeric format-version string is passed. In CreateTableStmt.java we've used Ints.tryParse(fv.trim()) with a null check. 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 GEOMETRY: Should we also add a GEOGRAPHY case in here, since its present in HiveSchemaUtil.java too? 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: // TODO: IMPALA-15162 will add GEOMETRY column support (Iceberg/Parquet) stale todo. 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 non-Iceberg tables via ALTER. -- 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: 6 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 22 Jul 2026 04:44:21 +0000 Gerrit-HasComments: Yes
