dramaticlly opened a new pull request, #17638:
URL: https://github.com/apache/iceberg/pull/17638
Reduces visibility on the scan-planning response internals that were
deprecated in 1.11.0 with visibility to be reduced in 1.12.0.
`.palantir/revapi.yml`: add 9 entries with
- 4 `noLongerDeprecated`
- 4 `visibilityReduced`
- 1 `method.removed`.
### 1. Reduce visibility
The specs map and derived delete files are serialization internals, not part
of the response payload:
- `specsById()` on the response and on `Builder` → `protected`
- `Builder.withSpecsById(Map)` → `protected`
- `Builder.deleteFiles()` → `protected`
- `Builder.withDeleteFiles(List)` **removed** — delete files are always
derived from the tasks that reference them
Since `withSpecsById` is no longer publicly callable, servers need another
way to supply the specs and response copiers need a way to carry them across:
- `builder(Map<Integer, PartitionSpec>)` on `PlanTableScanResponse`,
`FetchPlanningResultResponse` and `FetchScanTasksResponse`
- `toBuilder()` on `PlanTableScanResponse` and `FetchPlanningResultResponse`
`CatalogHandlers` passes `table.specs()` to `builder(...)` at the three
scan-planning call sites. `RESTServerCatalogAdapter` uses `toBuilder()` to
inject storage credentials rather than rebuilding field by field.
### 2. Clear derived delete files when file scan tasks are cleared
`BaseScanTaskResponse.Builder` derives `deleteFiles` from `fileScanTasks`,
but only refreshed the derived set when the incoming list was non-null:
```java
this.fileScanTasks = tasks;
if (fileScanTasks != null) {
this.deleteFiles = DeleteFileSet.of(...);
}
```
So `withFileScanTasks(null)` after a non-null call left the previously
derived delete files behind, and the response then failed its own `validate()`:
```
IllegalArgumentException: Invalid response: deleteFiles should only be
returned
with fileScanTasks that reference them
```
even though the caller had explicitly cleared the tasks. This is latent on
`main` but becomes load-bearing in commit 2, where `toBuilder()` makes builder
reuse routine — `TestRESTScanPlanning` now relies on exactly this path to turn
a COMPLETED response into a FAILED one.
Covered by two new tests at different entry points:
`TestFetchScanTasksResponseParser.clearingFileScanTasksAlsoClearsDerivedDeleteFiles`
(direct builder) and
`TestPlanTableScanResponseParser.toBuilderClearsDeleteFilesWhenClearingFileScanTasks`
(via `toBuilder()`). Both fail without the fix.
### Callout
1. I've added `toBuilder()` as a new public API for out-of-package
`org.apache.iceberg.rest` caller to copy a response with the specs.
1. Note `withCredentials` appends rather than replaces, so
`toBuilder().withCredentials(extra)` preserves any existing credentials; the
`TestRESTScanPlanning` simplification is behavior-preserving.
Split out of #16449 to reduce reviewer burden.
## AI Disclosure
Model: Claude Opus 5 (1M context)
Platform/Tool: Claude Code
Human Oversight: reviewed
Prompt Summary: split #16449 into smaller self-contained PRs; verify each
group compiles and tests green standalone
--
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]
