Revanth14 opened a new issue, #2102: URL: https://github.com/apache/iceberg-go/issues/2102
### Feature Request / Improvement Follow-up to #1178, per discussion with @laskoviymishka. Builds on #2094. ## Problem A REST catalog can return `scan-planning-mode` (`client` or `server`) in the load-table `config`. iceberg-go ignores it: scans default to local planning, and the README documents the gap. On a table that requires server planning, a default scan reads manifests with the table's own credentials. Those may belong to a different identity or lack the needed access, so the scan fails or bypasses the catalog's governance. #2094 exposes the value as `Table.ScanPlanningMode()` but doesn't enforce it. ## Java behavior `RESTSessionCatalog.restTableForScanPlanning`: - The client value comes from catalog properties; the server value comes from the load-table `config`. - If both are present and differ, Java logs a warning and the server value wins. - The effective mode is the server value, then the client value, then `client`. - If the effective mode is `server` and the catalog doesn't advertise `POST …/plan`, `LoadTable` fails. - In `server` mode the table plans remotely; otherwise it plans locally. ## Proposal Resolve the effective directive once, when the table is loaded, and apply it when a scan is planned. | Directive | Scan option | Result | |---|---|---| | `server` | none (default) | remote | | `server` | `ScanPlanningAuto` / `ScanPlanningRemote` | remote | | `server` | `ScanPlanningLocal` (explicit) | **error** | | `server`, plan endpoint not advertised | any | **error at `LoadTable`** (as in Java) | | `client` | none / `ScanPlanningAuto` / `ScanPlanningLocal` | local | | `client` | `ScanPlanningRemote` (explicit) | **error** | | absent | any | unchanged from today | ## Design questions 1. **An explicit option that conflicts with the directive.** Java has no per-scan option to compare against. Its client setting is catalog-wide and loses to the server with a warning. I propose an error for an explicit per-scan conflict, as the `ScanPlanningMode` doc comment already describes. The alternative is to follow Java: warn, and let the server win. 2. **Telling an explicit `local` apart from the default.** `Table.Scan` currently defaults to `ScanPlanningLocal`. That would need an unexported "unset" default, so an explicit `local` can be detected without changing the public constants. 3. **Where the directive comes from.** The current `scanPlanningConfig` merges catalog properties, table-metadata properties and the server's `config`. That matches Java's precedence, but it also lets a table property act as the directive, and it can't produce Java's mismatch warning. I propose resolving catalog properties and the server `config` separately, ignoring table properties, and logging the warning. 4. **Failing at `LoadTable` when the plan endpoint is missing.** This matches Java, but it also blocks metadata-only use of that table, such as reading the schema or committing. Is that acceptable? ## Scope - Resolve the directive where `table.go` has the `TODO(#1178 Phase 6): resolve scan-planning-mode table properties here` comment. - Rewrite the `ScanPlanningMode` doc comment in `table/scan_planning.go` to match. - Update the README "REST Scan Planning" section, removing the paragraph that says the directive is not honored. - Add unit tests for every row of the table above, and a REST catalog test with each `config` value. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
