Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/23455 )
Change subject: IMPALA-14108: Add support for SHOW FILES IN table PARTITION for Iceberg tables ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java: http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@48 PS3, Line 48: // FileWithPartition helper class removed - using lexicographic sorting in execution phase nit: unrelated to the change http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@137 PS3, Line 137: // To rewrite transforms and column references : IcebergPartitionExpressionRewriter rewriter = : new IcebergPartitionExpressionRewriter(analyzer, : table.getIcebergApiTable().spec()); : // For Impala expression to Iceberg expression conversion : IcebergPredicateConverter converter = : new IcebergPartitionPredicateConverter(table.getIcebergSchema(), analyzer); : : List<Expression> icebergPartitionExprs = new ArrayList<>(); : for (Expr expr : partitionSet_.getPartitionExprs()) { : expr = rewriter.rewrite(expr); : expr.analyze(analyzer); : analyzer.getConstantFolder().rewrite(expr, analyzer); : try { : icebergPartitionExprs.add(converter.convert(expr)); : } catch (ImpalaException e) { : throw new AnalysisException( : "Invalid partition filtering expression: " + expr.toSql()); : } : } This is a copy from AlterTableDropPartitionStmt, it could be moved to a common file like IcebergUtils. Also the above code that fetches data and delete files from the scan tasks could be extracted as a common method that accepts a consumer that contains the logic for how each scan task is processed. http://gerrit.cloudera.org:8080/#/c/23455/3/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/23455/3/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@493 PS3, Line 493: rowBuilder.add("Unknown"); // Size - could be enhanced later It could be populated by passing a compound object instead of a string to the catalog. IcebergContentFileStore is a possible alternative to pass file descriptors. If it's too complicated, a follow-up ticket could be created to solve the size display. http://gerrit.cloudera.org:8080/#/c/23455/3/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/23455/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4306 PS3, Line 4306: partition nit: 'partition' could be moved to the prefixes, as they are always in the query http://gerrit.cloudera.org:8080/#/c/23455/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test: http://gerrit.cloudera.org:8080/#/c/23455/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test@181 PS3, Line 181: nit: extra whitespace -- To view, visit http://gerrit.cloudera.org:8080/23455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb9913e078e6842861bdbb004ed5d67286bd3152 Gerrit-Change-Number: 23455 Gerrit-PatchSet: 3 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Thu, 25 Sep 2025 13:27:20 +0000 Gerrit-HasComments: Yes
