You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dramaticlly (via GitHub)" <gi...@apache.org> on 2026/08/13 17:59:45 UTC
[PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #17638:
URL: https://github.com/apache/iceberg/pull/17638
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#issuecomment-5350304623
>Note withCredentials appends rather than replaces, so toBuilder().withCredentials(extra) preserves any existing credentials; the TestRESTScanPlanning simplification is behavior-preserving.
This is actually quite surprising imo. I know it's an existing behavior but I feel like that's definitley not expected from a builder API; I forget if this was intentional or not, maybe @singhpk234 remembers. I guess it's not an issue hit in practice but I feel like we should have this replace credentials, and if we have a need for adding, we could do an `addCredentials`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#issuecomment-5427448857
I'll go ahead and merge. Thanks @dramaticlly !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#discussion_r3779265827
##########
core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java:
##########
@@ -85,28 +81,15 @@ public B withPlanTasks(List<String> tasks) {
public B withFileScanTasks(List<FileScanTask> tasks) {
this.fileScanTasks = tasks;
- if (fileScanTasks != null) {
- this.deleteFiles =
- DeleteFileSet.of(
- () -> tasks.stream().flatMap(task -> task.deletes().stream()).iterator());
- }
+ this.deleteFiles =
Review Comment:
Why is this changing?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#issuecomment-5350417806
> >Note withCredentials appends rather than replaces, so toBuilder().withCredentials(extra) preserves any existing credentials; the TestRESTScanPlanning simplification is behavior-preserving.
>
>
>
> This is actually quite surprising imo. I know it's an existing behavior but I feel like that's definitley not expected from a builder API; I forget if this was intentional or not, maybe @singhpk234 remembers. I guess it's not an issue hit in practice but I feel like we should have this replace credentials, and if we have a need for adding, we could do an `addCredentials`.
Thanks @amogh-jahagirdar , once we figured out the expected behaviour, happy to follow up and give a pass for all builder method deal with collections.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#issuecomment-5284498508
FYI @amogh-jahagirdar if you can help check against #14838
and also @singhpk234 for #14485
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#discussion_r3779398113
##########
core/src/main/java/org/apache/iceberg/rest/responses/BaseScanTaskResponse.java:
##########
@@ -85,28 +81,15 @@ public B withPlanTasks(List<String> tasks) {
public B withFileScanTasks(List<FileScanTask> tasks) {
this.fileScanTasks = tasks;
- if (fileScanTasks != null) {
- this.deleteFiles =
- DeleteFileSet.of(
- () -> tasks.stream().flatMap(task -> task.deletes().stream()).iterator());
- }
+ this.deleteFiles =
Review 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 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Reduce visibility on scan planning response builder for specsById and deleteFiles [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #17638:
URL: https://github.com/apache/iceberg/pull/17638#issuecomment-5427449907
and thanks @uros-b @RussellSpitzer for reviewing!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org