Mihaly Szjatinya 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 4: (14 comments) Thank you http://gerrit.cloudera.org:8080/#/c/23455/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23455/3//COMMIT_MSG@12 PS3, Line 12: a : NullPo > a NullPointerException. Ack http://gerrit.cloudera.org:8080/#/c/23455/3//COMMIT_MSG@25 PS3, Line 25: Testing: > Please check the possibility of adding an authorization test like https://g Added some tests, it takes some additional privileges for filtered syntax. Pls check. http://gerrit.cloudera.org:8080/#/c/23455/3/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/23455/3/common/thrift/Frontend.thrift@304 PS3, Line 304: nalysis phase > This field can be useful for other table formats in the future, e.g. Hudi, Done 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: /** > nit: unrelated to the change Ack http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@99 PS3, Line 99: new AnalysisException(String.format( > See AlterTableStmt L90-97 Ack 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 com I've tried to do it but it produced more java code than it is right now. I'd suggest to keep it as is until we have at least one more usage IYDM. http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@168 PS3, Line 168: String path = deleteFile.pa > This if statement unnecessarily encloses the inner for-loop. If deleteFiles Ack http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@169 PS3, Line 169: > nit: we could just have 'fileScanTask.deletes()' here, no need to introduce Ack http://gerrit.cloudera.org:8080/#/c/23455/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@179 PS3, Line 179: > I don't think it hurts too much, but this will be always true. Each fileSca Let's keep it safer then. 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: TResultRowBuilder rowBuilder = new TResultRowBuilder(); > We can retrieve the file descriptors here, IcebergUtil has @Zoltan, thank, it works. 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 yeah, I'd leave it explicit IYDM though 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@33 PS3, Line 33: day(day_date), day(da > nit: fits earlier line, but for readability it would be even better to put Ack http://gerrit.cloudera.org:8080/#/c/23455/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test@106 PS3, Line 106: Ack, applied where relevant. 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 ack -- 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: 4 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sat, 04 Oct 2025 13:54:41 +0000 Gerrit-HasComments: Yes
