You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2026/08/04 11:13:28 UTC
[Impala-ASF-CR] IMPALA-15200: Add Parquet UUID read support for Iceberg tables
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 <ar...@gmail.com>
Gerrit-Reviewer: Arnab Karmakar <ar...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 04 Aug 2026 11:13:28 +0000
Gerrit-HasComments: Yes