Csaba Ringhofer 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 19: (15 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 and the ALTER TABLE s > Can include AlterTableAlterColStmt and AlterTableReplaceColsStmt Done http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG@54 PS17, Line 54: und-tri > nit: don't Done http://gerrit.cloudera.org:8080/#/c/24536/17//COMMIT_MSG@56 PS17, Line 56: > nit: typo Done 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.thrift.TAccessEvent; > nit: can move the import up along with other catalog imports. Done 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.DATE, Pr > nit: we can format this better Done 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: ColumnStatisticsObj colStatsObj = new ColumnStatisticsObj(colName, : tableCol.getType().toHiveMetastoreType(), colStatsData); > can use String hmsColType = tableCol.getType().toHiveMetastoreType(); Done 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: ormalizes a case-insen > https://github.com/apache/iceberg/blob/e050d4d798d2241c40a6caf795d5098cea24 Done http://gerrit.cloudera.org:8080/#/c/24536/17/fe/src/main/java/org/apache/impala/util/IcebergSchemaConverter.java@254 PS17, Line 254: } > can be merged with binary type above. Done 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: * A maven-shade-plugin filter in the pom excludes Iceberg's own HiveSchemaUtil class > I think this is out of date as pom now explicitly excludes upstream's class Done 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_Linestring a > Not used in the insert below. Done http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test@184 PS17, Line 184: ---- QUERY : # IMPALA-15162: SHOW CREATE TABLE shows the GEOMETRY column type. : SHOW CREATE TABLE iceberg_geom_ > Unused table. Done 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 D Thanks for spotting this! There were 2 issue with these: 1. the logic to reject returning GEOMETRY to client also made rejected the inner query in these statement 2. update's optimization to skip rows where all SET columns are equal did not work due GEOMETRY being non-comperable Fixed 1, disabled the optimization in 2 for GEOMETRYs. + added tests Note that these didn't come up for other types as there were no types that are writable to tables, but are not returnable to clients or are not comparable. 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 ( > We can also check MAP along with ARRAY and STRUCT. Done http://gerrit.cloudera.org:8080/#/c/24536/17/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-geometry.test@95 PS17, Line 95: ==== > I think checking s too would make this stronger. Done 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 not > is this redundant as there's an assert checking geom_stats is not None on l 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: 19 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 08:19:05 +0000 Gerrit-HasComments: Yes
