Repository navigation
[#11196] feat(maintenance): Add builtin-iceberg-rewrite-manifests job - #11216
Open
ibrahimErbilen wants to merge 2 commits into
Open
ibrahimErbilen wants to merge 2 commits into
ibrahimErbilen wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new built-in maintenance job IcebergRewriteManifestsJob that runs Iceberg's rewrite_manifests procedure via Spark, along with its registration in the provider and unit tests.
Changes:
- New
IcebergRewriteManifestsJobclass implementingBuiltInJobwith arg parsing and SQL procedure call building. - Registration of the new job in
BuiltInJobTemplateProvider. - Unit tests covering job template metadata, argument parsing, and procedure call building.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergRewriteManifestsJob.java | New job implementation for Iceberg manifest rewriting. |
| maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/BuiltInJobTemplateProvider.java | Registers the new job in the built-in jobs list. |
| maintenance/jobs/src/test/java/org/apache/gravitino/maintenance/jobs/iceberg/TestIcebergRewriteManifestsJob.java | Unit tests for the new job. |
| new SparkPiJob(), | ||
| new IcebergRewriteDataFilesJob(), | ||
| new IcebergUpdateStatsAndMetricsJob()); | ||
| new IcebergUpdateStatsAndMetricsJob()), |
Comment on lines
31
to
+33
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.apache.gravitino.maintenance.jobs.iceberg.IcebergRewriteManifestsJob; |
Comment on lines
+126
to
+134
| Row[] results = (Row[]) spark.sql(sql).collect(); | ||
|
|
||
| if (results.length > 0) { | ||
| Row result = results[0]; | ||
| System.out.printf( | ||
| "Rewrite Manifests Results:%n" | ||
| + " Rewritten manifests: %d%n" | ||
| + " Added manifests: %d%n", | ||
| result.getInt(0), result.getInt(1)); |
Comment on lines
+184
to
+189
| if (i + 1 < args.length && !args[i + 1].startsWith("--")) { | ||
| String value = args[i + 1]; | ||
| if (value != null && !value.trim().isEmpty()) { | ||
| argMap.put(key, value); | ||
| } | ||
| i++; |
Comment on lines
+67
to
+68
| assertNotNull(template.className()); | ||
| assertEquals(IcebergRewriteManifestsJob.class.getName(), template.className()); |
Comment on lines
+107
to
+108
| Map<String, String> customConfigs = | ||
| IcebergRewriteDataFilesJob.parseCustomSparkConfigs(sparkConfJson); |
Collaborator
|
@ibrahimErbilen Could you fix this the copilot's comment? |
Collaborator
|
@ibrahimErbilen thanks for this work. Since the branch had been idle for a while and had drifted from Your two commits are preserved there with your authorship intact, and you are credited as co-author on the follow-up commit, so the squashed merge will attribute it to you as well. On top of the rebase I addressed the review comments here: the Happy to hand this back if you would rather finish it yourself - just say so and I will close mine. Otherwise this one can be closed in favour of #12937. Review comments on the new PR are very welcome either way. |
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>
laserninja
added a commit
to laserninja/gravitino
that referenced
this pull request
Sep 24, 2026
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>
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>
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
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?
builtin-iceberg-rewrite-manifests.IcebergRewriteManifestsJobto execute Iceberg'srewrite_manifestsaction via Spark's SQL procedure (CALL catalog.system.rewrite_manifests).--catalog,--table, and an optional--use-cachingargument.Why are the changes needed?
Over time, Iceberg tables accumulate many small manifest files from frequent commits. This slows down scan planning because the engine must open and read each manifest to determine which data files match a query filter.
Rewriting manifests into fewer, larger files with better-aligned partition specs significantly improves query planning performance. While
IcebergRewriteDataFilesJobaddresses data file layout, this new job specifically addresses manifest file bloat, enabling automatic policy-driven manifest optimization on the server side.Fix: #11196
Does this PR introduce any user-facing change?
Yes. Users can now run the
builtin-iceberg-rewrite-manifestsjob via the Gravitino API to optimize Iceberg table manifest files.How was this patch tested?
IcebergRewriteManifestsJobto verify argument parsing and Spark SQL procedure string generation.