Core: Reduce visibility on scan planning response builder for specsById and deleteFiles - #17638
Open
dramaticlly wants to merge 2 commits into
Open
Core: Reduce visibility on scan planning response builder for specsById and deleteFiles#17638dramaticlly wants to merge 2 commits into
dramaticlly wants to merge 2 commits into
Conversation
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.
Contributor
Author
|
FYI @amogh-jahagirdar if you can help check against #14838 |
| DeleteFileSet.of( | ||
| () -> tasks.stream().flatMap(task -> task.deletes().stream()).iterator()); | ||
| } | ||
| this.deleteFiles = |
Contributor
Author
There was a problem hiding this comment.
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.
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.
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 withnoLongerDeprecatedvisibilityReducedmethod.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 onBuilder→protectedBuilder.withSpecsById(Map)→protectedBuilder.deleteFiles()→protectedBuilder.withDeleteFiles(List)removed — delete files are always derived from the tasks that reference themSince
withSpecsByIdis 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>)onPlanTableScanResponse,FetchPlanningResultResponseandFetchScanTasksResponsetoBuilder()onPlanTableScanResponseandFetchPlanningResultResponseCatalogHandlerspassestable.specs()tobuilder(...)at the three scan-planning call sites.RESTServerCatalogAdapterusestoBuilder()to inject storage credentials rather than rebuilding field by field.2. Clear derived delete files when file scan tasks are cleared
BaseScanTaskResponse.BuilderderivesdeleteFilesfromfileScanTasks, but only refreshed the derived set when the incoming list was non-null:So
withFileScanTasks(null)after a non-null call left the previously derived delete files behind, and the response then failed its ownvalidate():even though the caller had explicitly cleared the tasks. This is latent on
mainbut becomes load-bearing in commit 2, wheretoBuilder()makes builder reuse routine —TestRESTScanPlanningnow 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) andTestPlanTableScanResponseParser.toBuilderClearsDeleteFilesWhenClearingFileScanTasks(viatoBuilder()). Both fail without the fix.Callout
I've added
toBuilder()as a new public API for out-of-packageorg.apache.iceberg.restcaller to copy a response with the specs.Note
withCredentialsappends rather than replaces, sotoBuilder().withCredentials(extra)preserves any existing credentials; theTestRESTScanPlanningsimplification 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