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 13: (8 comments) http://gerrit.cloudera.org:8080/#/c/24536/12/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/12/be/src/exec/parquet/hdfs-parquet-table-writer.cc@191 PS12, Line 191: parquet::Statistics stats; > Is this where the issue is with incorrectly written stats? Do they still n PS 13 rewritten stat handling null count is still need, which is handled by the geometry specific branch, while other types write min/max too http://gerrit.cloudera.org:8080/#/c/24536/12/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/12/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java@682 PS12, Line 682: // representation (no NDV; null count + byte size). > Should we consider adding NDV stats at a later time? IMO no, NDV is not useful for geometry - they don't have an = operator, so ndv can't help with estimates for predicates or joins. http://gerrit.cloudera.org:8080/#/c/24536/12/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/12/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4378 PS12, Line 4378: AnalysisError("create table new_table (g geometry) stored as iceberg " : + "tblproperties('format-version'='2')", > This DDL should specify an Iceberg table version of 2 so it does not start Done http://gerrit.cloudera.org:8080/#/c/24536/12/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4469 PS12, Line 4469: + "tblproperties('format-version'='3')"); : // Nested GEOMETRY still requires format-version >= 3. : AnalysisError("create table geom_ice_nested (id int, arr array<geometry>) " : + "stored as iceberg tblproperties('format-version'='2')", : "GEOMETRY is only supported for Iceberg tables with format version 3 or higher"); : // Nested GEOMETRY is rejected on a non-Iceberg table. > These DDLs should specify an Iceberg table version of 2 so it does not star Done http://gerrit.cloudera.org:8080/#/c/24536/12/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4484 PS12, Line 4484: "GEOMETRY is only supported for Iceberg tables with format version 3 or higher"); : AnalysisError("create table geom_ice_ctas stored as iceberg " : + "tblproperties('format-version'='2') as select cast(NULL as geometry) as g", : "GEOMETRY is only supported for Iceberg tables with format version 3 or higher"); : // Non-Iceberg GEOMETRY columns are rejected. : AnalysisError("create table geom_plain (id int, g geometry)", > This DDL should specify an Iceberg table version of 2 so it does not start Done http://gerrit.cloudera.org:8080/#/c/24536/12/fe/src/test/java/org/apache/impala/util/IcebergHiveSchemaUtilTest.java File fe/src/test/java/org/apache/impala/util/IcebergHiveSchemaUtilTest.java: http://gerrit.cloudera.org:8080/#/c/24536/12/fe/src/test/java/org/apache/impala/util/IcebergHiveSchemaUtilTest.java@1 PS12, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > Should this class be moved to the java/shaded-deps/impala-iceberg-runtime/ I think no, adding tests for the shaded dep is extra effort. http://gerrit.cloudera.org:8080/#/c/24536/12/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/12/java/shaded-deps/impala-iceberg-runtime/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java@116 PS12, Line 116: case BINARY: > Nit: trailing space Done http://gerrit.cloudera.org:8080/#/c/24536/12/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/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-geometry.test@206 PS12, Line 206: '{1:"AQAAAA=="}','{1:"AgAAAA=="}' > Hardcode table version here to prevent test case failure when Iceberg defau removed the test -- 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: 13 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: Mon, 28 Sep 2026 08:17:53 +0000 Gerrit-HasComments: Yes
