Repository navigation
[#11196] feat(maintenance): Add builtin-iceberg-rewrite-manifests job - #12937
Open
laserninja wants to merge 8 commits into
Open
laserninja wants to merge 8 commits into
laserninja wants to merge 8 commits into
Conversation
Rebases the job onto current main and resolves the review comments left on apache#11216: - Fix the misplaced closing parenthesis in BuiltInJobTemplateProvider that left the new job outside ImmutableList.of(...) and broke compilation, and restore alphabetical import ordering. - Use IcebergJobUtils for SQL escaping, argument parsing and Spark config parsing instead of reaching into IcebergRewriteDataFilesJob, and drop the duplicated parseArguments implementation. - Drop unresolved {{...}} template placeholders for optional arguments. The server forwards them verbatim when a jobConf key is absent, so use_caching => {{use_caching}} silently parsed to false instead of leaving Iceberg's default in place. - Reject a use-caching value that is neither true nor false rather than letting Boolean.parseBoolean turn a typo into false. - Read the procedure output through collectAsList and Number so the job does not depend on whether the runtime Iceberg version types the manifest counts as int or long. The Iceberg jar is a compileOnly dependency supplied by the cluster, so that width can differ from what we compile against. - Update the SQL assertions for backtick-quoted catalog identifiers, which is what escapeSqlIdentifier now produces, and rename testJobTemplateHasMainClass to match the property it asserts on. - Document the job in the table maintenance CLI reference alongside the other three built-in templates. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Collaborator
|
Could u write a design document for th rewrite mainfests job? |
Iceberg's rewrite_manifests procedure takes three in-parameters: table, use_caching, and spec_id. Only the first two were reachable, which left the "align manifests with a better partition spec" half of apache#11196 unavailable - a spec evolution had no way to re-cluster old manifests onto the new spec. Expose it as jobConf key spec_id, validated as a non-negative integer so a malformed value fails before the Spark session is created rather than reaching Spark as an unparseable SQL literal. Like use_caching, an unresolved {{spec_id}} placeholder is dropped so an omitted parameter leaves the table's current spec in force. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Collaborator
Author
|
@lasdf1234 done - #12942, following the convention that every design doc lands as its own PR. It covers why manifest bloat is distinct from data-file bloat and why neither Two things changed on this PR while writing it:
|
Code Coverage Report
Files
|
Document spec discovery and correct CLI examples and help text without changing execution behavior. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Use matching argument placeholders and the catalog key in Spark configuration while preserving defaults for existing jobs. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Revert CLI/key alignment after the review restriction was withdrawn. Restore the shared catalog helper and original tests and examples. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
roryqi
pushed a commit
that referenced
this pull request
Sep 18, 2026
…12942) ### What changes were proposed in this pull request? Design for `builtin-iceberg-rewrite-manifests`, following the numbered architecture and layer structure of #11700. Covers template registration, procedure parameters, spec ID discovery, execution, safety, and the implementation plan. The initial job is submitted directly. Follow-up statistics are limited to `custom-manifest-number-by-spec` and `custom-avg-manifest-size-by-spec`; automatic policy triggering remains separate implementation work. The design defines defaults of 500 manifests (critical), or 100 manifests with average size below 8 MiB, and preserves one resolved spec ID across collection, evaluation, and submission. Initial CI/runtime support is Spark 3.5 with Iceberg 1.11.0. Related: #11196, #12937 (implementation), #8864. Continues the original work by @ibrahimErbilen in #11216, with co-authorship retained. ### Why are the changes needed? Operators need a dedicated manifest maintenance job and clear guidance after partition evolution. The document explains that `spec_id` selects manifests belonging to an existing spec; it does not migrate data files between specs. Includes the Iceberg 1.11.0 source supporting that behavior. ### Does this PR introduce _any_ user-facing change? No runtime changes. User-facing documentation is maintained in #12937. ### How was this patch tested? - Checked all Markdown tables for aligned columns and balanced code fences. - Verified spec selection and caching defaults against Iceberg 1.11.0 source. - `git diff --check`. - `./gradlew rat` passed; companion CLI documentation passed `./gradlew :docs:build`. --------- Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
roryqi
pushed a commit
that referenced
this pull request
Sep 28, 2026
…13329) ### What changes were proposed in this pull request? Implements the first phase of the merged design in #12942: the `builtin-iceberg-rewrite-manifests` Spark job, registration, validated arguments, spec selection, and operator documentation. Manifest statistics and the Policy -> Strategy -> Adapter integration for automatic triggering remain separate follow-ups in the design, as requested in review. Continues #12937 on current main and preserves the original commits by @ibrahimErbilen from #11216, with co-author attribution on follow-up work. Adds Spark-backed tests invoking the job entry point through resolved template arguments. They verify default and explicit partition-spec selection, unchanged table records and other-spec manifests, repeat-run no-op behavior, and cleanup without metadata changes after an unknown-spec failure. The entry point checks the Iceberg runtime, closes Spark on success or failure, prints user-visible results to stdout for output.log, and reports concise errors to stderr with a nonzero exit. Strict argument parsing is shared through IcebergJobUtils while preserving existing callers. The overview and configuration guides document direct submission and the remaining policy integration. ### Why are the changes needed? Manifest rewriting maintains manifest layout independently of data-file compaction. The previous implementation lacked Spark-backed verification of partition-spec selection and failure cleanup. Fix: #11196 ### Does this PR introduce _any_ user-facing change? Adds `builtin-iceberg-rewrite-manifests` with required `catalog_name` and `table_identifier`, plus optional `use_caching`, `spec_id`, and `spark_conf`. Existing jobConf/CLI naming conventions are retained. Invalid, duplicate, or missing argument values fail before session creation; omitted optional values retain Iceberg defaults. `spec_id` selects an existing spec, without migrating files between specs. ### How was this patch tested? - 183 jobs-module tests passed, including 33 manifest unit tests and 3 Spark-backed tests on Spark 3.5 / Iceberg 1.11.0. - `./gradlew :maintenance:jobs:test :maintenance:jobs:spotlessCheck :maintenance:jobs:javadoc :docs:build rat -PskipITs` passed with JDK 17. - Process-level checks verified exit code 1, concise stderr, and no stdout or stack trace for missing arguments and malformed Spark configuration. - `git diff --check` passed. Spark tests use a local Hadoop catalog; remote catalog deployment and the server REST submission path were not exercised end to end. --------- Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Collaborator
|
@roryqi This is a split PR that has been reviewed several times. This PR seems fine for me. If you have time, please take a look. Thank you. |
Resolve conflicts by retaining the implementation merged in apache#13329 and subsequent upstream improvements. The resulting tree matches upstream main.
roryqi
self-requested a review
October 16, 2026 06:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Adds
builtin-iceberg-rewrite-manifests, invoking Iceberg's Spark SQL procedure withtable, optionaluse_caching, and optionalspec_id. Registers the template, reuses shared job utilities, validates parameters, handles omitted optional arguments, and reports rewritten/added counts.Continues #11216 by @ibrahimErbilen. Their original commits retain authorship, and follow-up commits retain their co-author credit. Design: #12942.
Why are the changes needed?
Manifest maintenance consolidates and clusters manifest entries independently of data-file compaction.
spec_idselects an existing spec's manifests; it does not repartition files or migrate manifests from another spec.Fix: #11196
Does this PR introduce any user-facing change?
New job template with
catalog_name,table_identifier,use_caching,spec_id, andspark_confconfiguration. The CLI reference explains how to discover spec IDs from Iceberg metadata and includes a day-to-hour evolution example. Omitted caching uses the installed Iceberg default.How was this patch tested?
:docs:build,:maintenance:jobs:spotlessCheck, andgit diff --check.A dedicated Spark-backed test for this job is still pending; the module's existing integration checks do not establish its spec-selection behavior end to end.