Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/24979 )
Change subject: IMPALA-13704: Support OPTIMIZE TABLE for selected partitions ...................................................................... Patch Set 2: (17 comments) Thank you very much for the review comments! http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@11 PS1, Line 11: expr_list > Is it possible to have a more complex expression, e.g. i>1? I only saw = in Yes, it accepts the same predicates as DROP PARTITION, because they share the selection logic. Added tests. http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@14 PS1, Line 14: other partitions are left untouched. > Puffin files can contain Deletion Vectors referring to files from different Fixed the delete vector selection/removal. Added a test with a generated Puffin file that contains DVs from multiple partitions. http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@16 PS1, Line 16: Details: > Can you describe how partition evolution is handled? Can you optimize a par Done http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@36 PS1, Line 36: what engines that write one Puffin file per writer task produce. The > nit: weird line break Done http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@37 PS1, Line 37: commit therefore removes a deletion vector if and only if the data file : it belongs to is replaced, and files_to_replace lists no Puffin paths. : An equality delete file written > Is this setup really relevant? If we bump the CatalogServiceVersion at the same time, then no. http://gerrit.cloudera.org:8080/#/c/24979/1//COMMIT_MSG@56 PS1, Line 56: At finalization, PARTIAL mode sends the catalog the paths of every data > Please add e2e tests for partition transforms, partition evolution, V3 tabl Done 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: ergTable) table_; > It feels a bit weird to refer to OPTIMIZE here. I think the comment should Done 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: // END: Members that need to be reset() : ///////////////////////////////////////// : : /** : * 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 the selected files this mode replaces: data files with : // and without deletes, position delete files and equality delete files. : // Deletion vectors are deliberately not listed. A DV lives in a Puffin file that may : // also hold the DVs of data files in other partitions, so the catalog cannot remove : // DVs by location; it removes a DV iff the data file it belongs to is replaced, and : // that data file is in this set by construction. : 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; } : : // RewriteAll replaces the whole table, so no need to define the file selection. : @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 the files the commit must remove by path. : // Deletion vectors are not included; see OptimizeMode.getFilesToReplace(). : 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_; } : : > These are a lot of lines for very little and simple logic. So I'm not sure I hope it creates a cleaner interface. http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/analysis/OptimizeStmt.java@379 PS1, Line 379: > Building the scoped store decodes every file descriptor in the table and bu Good idea, thanks! 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. Done 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: ataFilesToAdd = ice > It could return the number of matches which could be checked against files_ Done http://gerrit.cloudera.org:8080/#/c/24979/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@768 PS1, Line 768: o remove. Re > Multiple Deletion Vectors (from different partitions) can be stored in a si Done http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-optimize.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-optimize.test: http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-optimize.test@161 PS1, Line 161: 03:SORT : | order by: action ASC NULLS LAST : | mem-estimate=12.00MB mem-reservation=12.00MB spill-buffer=2.00MB thread-reservation=0 : | tuple-ids=4 row-size=44B cardinality=3 : | in pipelines: 03(GETNEXT), 00(OPEN) > Why do we have a sort in this case? Currently, we always sort for partitioned Iceberg tables. See IMPALA-12995 http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test: http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test@326 PS1, Line 326: PARTITIONED BY SPEC(i) > Does optimize work for other partitioned spec than identity? Yes, added tests. http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test@382 PS1, Line 382: ---- LABELS > Why does MAX_FS_WRITERS matter here? The optimize won't add shuffle to crea It matters only for unpartitioned tables, no change in that. Fixed. http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test@397 PS1, Line 397: > here and at other places: order by is not needed Created a IMPALA-15482 for the .md update. http://gerrit.cloudera.org:8080/#/c/24979/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-optimize.test@408 PS1, Line 408: > Is there any test where FILE_SIZE_THRESHOLD_MB actually leads to keeping a FILE_SIZE_THRESHOLD_MB is an int, so it cannot compact below 1MB. Added a test table generated from lineitem to check file size filtering. -- 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: 2 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[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: Wed, 07 Oct 2026 14:46:28 +0000 Gerrit-HasComments: Yes
