Arnab Karmakar 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 8: (8 comments) 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: return new ScalarColumnReader<StringValue, parquet::Type::BYTE_ARRAY, true>( : parent, nod > This can cause NULL-deref in release builds when logicalType is missing. Le Done. We are validating it in ParquetMetadataUtils::ValidateColumn(). 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 < 0 || end_idx < start_idx > This doesn't seem to be right. How about: Done 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( Done 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: on { > Can we use Type.containsUuid() instead of just checking the top-level type? Done http://gerrit.cloudera.org:8080/#/c/24505/7/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@157 PS7, Line 157: for (Column col : getColumns()) { : if (col.isHidden() || !col.getType().containsUuid()) continue; : hasUuid = true; : break; : } : if (!hasUuid) return; : : Set > Instead of having a deny-list, we should have an allow-list (with only Parq We are now traversing through all formats in HdfsScanNode.fileFormats_ and allowing only parquet file format. 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 us Great suggestion. Done. 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. Right but I feel these test cases are not going to add any value. We already have other positive and negative EE test cases for reading against diff file formats. I can put this in if you feel it is needed. 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 Done. 1. SELECT DISTINCT is causing impalad crash, fixed it by disabling hash-table codegen like TYPE_CHAR. Interpreted path works via RawValue memcmp/hash. 2. IS NULL is also failing because IsNullPredicate doesn't check for a missing builtin, so analysis passes and the backend fails. Adding the missing null check. -- 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: 8 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: Mon, 17 Aug 2026 08:53:47 +0000 Gerrit-HasComments: Yes
