lasdf1234 commented on PR #13533: URL: https://github.com/apache/gravitino/pull/13533#issuecomment-6381132834
## Feedback: `spec_id` should be optional (resolve Iceberg `default-spec-id`) Requiring an explicit policy `spec_id` for the recommender/CLI is the main product gap left on this PR. Iceberg already defines the right default; we should match that instead of skipping when the field is omitted. ### Expected behavior (same as Iceberg) | Policy `spec_id` | Behavior | | --- | --- | | Set | Evaluate / submit that existing spec (e.g. rewrite old-spec manifests after partition evolution) | | Omitted | Resolve the table’s **current** `default-spec-id`, then evaluate and submit that ID | | Unresolvable | Warn + skip that table (do not abort the whole recommend batch) | Whatever ID is used for evaluation must be **pinned** into `jobConf` (`ManifestRewriteJobContext`). Do not omit it on submit and let Iceberg re-resolve later — partition evolution between evaluate and submit would otherwise retarget the job. ### Suggested wiring (no user-facing Gravitino config) This is **not** a new Gravitino property operators set. Source of truth is Iceberg `TableMetadata.defaultSpecId()`. The recommender today only sees `org.apache.gravitino.rel.Table` via `StrategyHandlerContext` (same path as the snapshot fence). The smallest change that fits existing patterns: 1. Project Iceberg’s `default-spec-id` into reserved table properties in `IcebergTablePropertiesUtil` / `IcebergConstants`, the same way `current-snapshot-id` is projected today (read-only passthrough from Iceberg metadata). 2. In `ManifestRewriteStrategyHandler`, when policy `spec_id` is null, parse that projected value; then run `fromStatistics(..., resolved)` and build `ManifestRewriteJobContext` with the same `resolved` int. 3. Update docs / OpenAPI / Javadoc: omitted = current default spec. Drop “recommender requires explicit `spec_id`”. Collapse or `@VisibleForTesting` the two-arg `initialize` so CLI and tests don’t diverge. ### Why not keep “must set” - Iceberg Spark `rewrite_manifests` / `RewriteManifests.specId()` already default to the table default when omitted. - Hard-coding a spec in the policy goes stale after partition evolution; resolving at evaluate time stays correct for routine maintenance. - Empty / default system policy content should be able to trigger without operators looking up metadata IDs by hand. Happy to discuss if you’d rather give the handler a direct Iceberg `TableMetadata` handle instead of the property projection — either way the value must come from Iceberg, not from a user-written Gravitino property. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
