Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24505 )
Change subject: IMPALA-15200: Add Parquet UUID read support for Iceberg tables ...................................................................... Patch Set 7: (8 comments) Looks good overall! http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/exec/parquet/parquet-column-readers.cc File be/src/exec/parquet/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/exec/parquet/parquet-column-readers.cc@1912 PS7, Line 1912: DCHECK(false) << slot_desc->type().DebugString(); : return nullptr; This can cause NULL-deref in release builds when logicalType is missing. Let's require the annotation in ParquetMetadataUtils::ValidateColumn(). Or just accept it if len is 16. http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/exec/parquet/parquet-column-stats.cc File be/src/exec/parquet/parquet-column-stats.cc: http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/exec/parquet/parquet-column-stats.cc@387 PS7, Line 387: if (start_idx > end_idx || end_idx - start_idx + 1 > encoded_values.size()) return -1; This doesn't seem to be right. How about: if (start_idx < 0 || end_idx < start_idx || static_cast<size_t>(end_idx) >= encoded_values.size()) { return -1; } http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/runtime/uuid-value.h File be/src/runtime/uuid-value.h: http://gerrit.cloudera.org:8080/#/c/24505/7/be/src/runtime/uuid-value.h@36 PS7, Line 36: DCHECK_EQ(len, BYTE_SIZE); This does not protect release builds. ParquetMetadataUtils::ValidateColumn() should check the column's length in Parquet metadata. http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@155 PS7, Line 155: !col.getType().isUuid() Can we use Type.containsUuid() instead of just checking the top-level type? http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@157 PS7, Line 157: if (format == TIcebergFileFormat.ORC) { : throw new AnalysisException( : "Reading UUID columns from ORC format is not yet supported."); : } : if (format == TIcebergFileFormat.AVRO) { : throw new AnalysisException( : "Reading UUID columns from Avro format is not yet supported."); : } Instead of having a deny-list, we should have an allow-list (with only Parquet for now). So if TIcebergFileFormat extends in the future, we won't silently accept it. http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@677 PS7, Line 677: validateUuidReadSupported validateUuidReadSupported() uses only the default file format. We should use 'fileFormats_' (populated by IcebergScanNode.populateFileFormats) that lists all file formats in the table. We could have a Trino interop test for this. http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@a3519 PS7, Line 3519: : : : : : These could be AnalyzesOk now. http://gerrit.cloudera.org:8080/#/c/24505/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test: http://gerrit.cloudera.org:8080/#/c/24505/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test@43 PS7, Line 43: ==== Please add tests for * ORDER BY uuid_col * SELECT DISTINCT uuid_col * GROUP BY uuid_col * HAVING uuid_col * uuid_col IS NULL At least we should verify Impala doesn't crash and raises proper error messages for the cases we don't support yet. -- To view, visit http://gerrit.cloudera.org:8080/24505 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4157c002e80677d27d8fd060c7bfa07b95d7c78f Gerrit-Change-Number: 24505 Gerrit-PatchSet: 7 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 04 Aug 2026 11:13:28 +0000 Gerrit-HasComments: Yes
