Skip to content

Core: Reduce visibility on scan planning response builder for specsById and deleteFiles - #17638

Open
dramaticlly wants to merge 2 commits into
apache:mainfrom
dramaticlly:1.12deprecation-scan-planning-visibility
Open

Core: Reduce visibility on scan planning response builder for specsById and deleteFiles#17638
dramaticlly wants to merge 2 commits into
apache:mainfrom
dramaticlly:1.12deprecation-scan-planning-visibility

Conversation

@dramaticlly

Copy link
Copy Markdown
Contributor

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 Builderprotected
  • 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:

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.

  2. 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

BaseScanTaskResponse.Builder derives deleteFiles from fileScanTasks, but
only updated the derived set when the incoming list was non-null. Passing
null after a non-null call therefore left the previously derived delete
files in place, and the resulting response failed validate() with
"deleteFiles should only be returned with fileScanTasks that reference
them" even though the caller had cleared the tasks.

Derive deleteFiles unconditionally so it tracks fileScanTasks.
The specs map and derived delete files on BaseScanTaskResponse are
serialization internals, not part of the response payload. They were
deprecated in 1.11.0 with visibility to be reduced in 1.12.0:

- 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 file scan tasks that reference them

Because withSpecsById is no longer publicly callable, servers need another
way to supply the specs, and callers that copy a response need a way to
carry them across. Added:

- builder(Map<Integer, PartitionSpec>) on PlanTableScanResponse,
  FetchPlanningResultResponse and FetchScanTasksResponse
- toBuilder() on PlanTableScanResponse and FetchPlanningResultResponse,
  used by RESTServerCatalogAdapter to inject storage credentials into an
  already-built response instead of rebuilding it field by field

CatalogHandlers now passes table.specs() to builder(...) at the three
scan-planning call sites.
@dramaticlly

Copy link
Copy Markdown
Contributor Author

FYI @amogh-jahagirdar if you can help check against #14838
and also @singhpk234 for #14485

DeleteFileSet.of(
() -> tasks.stream().flatMap(task -> task.deletes().stream()).iterator());
}
this.deleteFiles =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this changing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @RussellSpitzer, basically we need a way to clean the deleteFiles after the withDeleteFiles setter has been removed.

Given the delete files can only be derived from from fileScanTasks, use withFileScanTasks(null) would allow clear the internal state of this.deleteFiles.

@nssalian nssalian added this to the Iceberg 1.12.0 milestone Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants