Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24979 )
Change subject: IMPALA-13704: Support OPTIMIZE TABLE for selected partitions ...................................................................... Patch Set 1: (8 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@14 PS1, Line 14: other partitions are left untouched. > Can you add note about delete files here? A delete file can only belong to Puffin files can contain Deletion Vectors referring to files from different partitions. E.g. Spark writes such Puffin files. We should add a test for this scenario by adding a Spark-written Iceberg V3 table. http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@56 PS1, Line 56: Please add e2e tests for partition transforms, partition evolution, V3 tables with DVs, or equality deletes. http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java: http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java@136 PS1, Line 136: used by OPTIMIZE TABLE ... PARTITION It feels a bit weird to refer to OPTIMIZE here. I think the comment should just mention "whole-partition resolution" http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java File fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java: http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@119 PS1, Line 119: /** : * Represents the mode of an OPTIMIZE operation that has an effect on the table. It is : * decided during analysis, once the set of files to rewrite is known (see : * selectFiles()), which is why it is modeled as an object instantiated at that point : * rather than as a flag set at construction time. : * : * The mode also owns the data that is specific to it: only {@link Partial} restricts : * the operation to a subset of the table, so only it carries the scoped file store the : * scan reads and the paths of the files the commit replaces. This keeps the "a : * selection exists only in PARTIAL mode" invariant type-enforced instead of spread : * across nullable fields and mode checks. : * : * The NOOP case is not represented here; it is tracked by : * {@link StatementBase#isNoOp()} and leaves mode_ null. : */ : private abstract static class OptimizeMode { : abstract TIcebergOptimizationMode toThrift(); : : // Iceberg file path strings of every file this mode replaces, across all content file : // categories. Empty for RewriteAll, which replaces the whole table without : // enumerating it. The catalog matches these against the locations of the files it : // re-plans at commit time and removes exactly the ones that match, so they must be : // Iceberg's own location strings (see IMPALA-15409) and the set must be complete: : // listing a path that was not scanned and rewritten would remove live data. : abstract Set<String> getFilesToReplace(); : : // File store the compaction scan should read, holding exactly the selected files. : // Null for RewriteAll, in which case the scan reads the table's own file store. : IcebergContentFileStore getScopedFileStoreForScan() { return null; } : } : : /** : * Rewrites all files of the table, ensuring the optimized table has the latest schema : * and partition spec. Used when neither FILE_SIZE_THRESHOLD_MB nor a PARTITION clause : * narrows the operation, or when the threshold is large enough that all files get : * selected. : */ : private static final class RewriteAll extends OptimizeMode { : @Override : TIcebergOptimizationMode toThrift() { return TIcebergOptimizationMode.REWRITE_ALL; } : : @Override : Set<String> getFilesToReplace() { return Collections.emptySet(); } : } : : /** : * Rewrites only the selected files, leaving all others unchanged. The scope is narrowed : * by a PARTITION clause (to the files of the selected partition(s)), by : * FILE_SIZE_THRESHOLD_MB (to the data files without deletes below the threshold), or by : * both. Within that scope all delete files and all data files with deletes are always : * rewritten so that the delete deltas get merged. Note that the use of the latest : * schema and partition spec for the entire table is not guaranteed in this mode. : */ : private static final class Partial extends OptimizeMode { : // File store the compaction scan reads. Holds exactly the selected files, in all : // content file categories. : private final IcebergContentFileStore scopedFileStore_; : // Iceberg locations of all the files the commit must remove, from : // ContentFile.location(). Must not be normalized through Hadoop Path/URI: that : // re-encodes the percent-escapes Iceberg puts in partition paths, so the catalog : // would match nothing (IMPALA-15409). : private final Set<String> filesToReplace_; : : Partial(IcebergContentFileStore scopedFileStore, Set<String> filesToReplace) { : scopedFileStore_ = scopedFileStore; : filesToReplace_ = filesToReplace; : } : : @Override : TIcebergOptimizationMode toThrift() { return TIcebergOptimizationMode.PARTIAL; } : : @Override : IcebergContentFileStore getScopedFileStoreForScan() { return scopedFileStore_; } : : @Override : Set<String> getFilesToReplace() { return filesToReplace_; } : } These are a lot of lines for very little and simple logic. So I'm not sure if it is needed when a simple if/else statement could do the work. http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@379 PS1, Line 379: IcebergContentFileStore scopedStore = new IcebergContentFileStore( Building the scoped store decodes every file descriptor in the table and builds a path map, even when the partition holds only a few files. Looking each file up by path hash in the table's existing store would avoid that. http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java: http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@266 PS1, Line 266: scopedStore = optimizeStmt.getScopedFileStoreForScan(); We could Precondition check that there are no missing files in scopedStore. http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@756 PS1, Line 756: deleteReplacedFiles It could return the number of matches which could be checked against files_to_replace.size() http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@768 PS1, Line 768: f.location() Multiple Deletion Vectors (from different partitions) can be stored in a single puffin file, se we should match a DV by referencedDataFile() being in the replaced set, not by its location. -- To view, visit http://gerrit.cloudera.org:8080/24979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I468f0bf5309ddce67047e3e5e52e60304967af10 Gerrit-Change-Number: 24979 Gerrit-PatchSet: 1 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 02 Oct 2026 08:27:08 +0000 Gerrit-HasComments: Yes
