Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24536 )
Change subject: IMPALA-15162: Add GEOMETRY column support for Iceberg and Parquet ...................................................................... Patch Set 17: (16 comments) http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG@22 PS17, Line 22: TableDef/AlterTableAddColsStmt Can include AlterTableAlterColStmt and AlterTableReplaceColsStmt http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG@54 PS17, Line 54: doesn't nit: don't http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG@56 PS17, Line 56: Icerberg nit: typo http://gerrit.cloudera.org:8080/#/c/24536/17/be/src/exec/parquet/hdfs-parquet-table-writer.cc File be/src/exec/parquet/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/24536/17/be/src/exec/parquet/hdfs-parquet-table-writer.cc@a516 PS17, Line 516: : I think we should keep this comment. http://gerrit.cloudera.org:8080/#/c/24536/17/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/17/fe/src/main/java/org/apache/impala/analysis/TableDef.java@45 PS17, Line 45: import org.apache.impala.catalog.IcebergTable; nit: can move the import up along with other catalog imports. http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java File fe/src/main/java/org/apache/impala/catalog/ColumnStats.java: http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java@60 PS17, Line 60: PrimitiveType.BOOLEAN, nit: we can format this better http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2281 PS17, Line 2281: String hmsColType = tableCol.getType().isGeometry() : ? "binary" : tableCol.getType().toString().toLowerCase(); can use String hmsColType = tableCol.getType().toHiveMetastoreType(); http://gerrit.cloudera.org:8080/#/c/24536/17/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/17/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@103 PS17, Line 103: geomType.crs() == null https://github.com/apache/iceberg/blob/e050d4d798d2241c40a6caf795d5098cea24b6d5/api/src/main/java/org/apache/iceberg/types/Types.java#L592-L606 I found that crs() wouldnt return null and fallsback to DEFAULT_CRS. http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@254 PS17, Line 254: if (impalaType.isGeometry()) { can be merged with binary type above. http://gerrit.cloudera.org:8080/#/c/24536/17/java/shaded-deps/impala-iceberg-runtime/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java File java/shaded-deps/impala-iceberg-runtime/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java: http://gerrit.cloudera.org:8080/#/c/24536/17/java/shaded-deps/impala-iceberg-runtime/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java@37 PS17, Line 37: * The maven-shade-plugin gives project classes precedence over dependency classes, I think this is out of date as pom now explicitly excludes upstream's class with a filter. http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test: http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test@67 PS17, Line 67: ST_GeomFromText Not used in the insert below. http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test@184 PS17, Line 184: ---- QUERY : # A plain (non-Iceberg) table used for the ALTER negative case below. : CREATE TABLE plain_tbl (id INT) Unused table. http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test: http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test@1 PS17, Line 1: # Impala <-> Trino interop over Apache Iceberg V3 tables: GEOMETRY columns. I tried running few queries with geometry type and found INSERT, CTAS and DELETE all work. But, UPDATE, MERGE and OPTIMIZE are unusable on GEOMETRY tables likely because their paths use the root analyzer. Is this behaviour intended as I couldn't see it mentioned anywhere? Maybe we should add some test cases for these statements too. http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test@85 PS17, Line 85: CREATE TABLE it_geom_nested (id integer, arr array(Geometry), s row(g Geometry, name varchar)) We can also check MAP along with ARRAY and STRUCT. http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test@95 PS17, Line 95: ---- RESULTS: VERIFY_IS_SUBSET I think checking s too would make this stronger. http://gerrit.cloudera.org:8080/#/c/24536/17/tests/query_test/test_geospatial_functions.py File tests/query_test/test_geospatial_functions.py: http://gerrit.cloudera.org:8080/#/c/24536/17/tests/query_test/test_geospatial_functions.py@97 PS17, Line 97: geom_stats is None is this redundant as there's an assert checking geom_stats is not None on line 100. -- 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: 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-Comment-Date: Wed, 30 Sep 2026 04:53:02 +0000 Gerrit-HasComments: Yes
