REST: Restrict visibility of spec-by-id API for scan planning responses#14485
Conversation
| new: "method java.util.Map<java.lang.Integer, org.apache.iceberg.PartitionSpec>\ | ||
| \ org.apache.iceberg.rest.responses.BaseScanTaskResponse::specsById()" | ||
| justification: "specById API is not part of Plan API response spec. No actual\ | ||
| \ breakage because it never worked before." |
There was a problem hiding this comment.
the because it never worked before seems rather confusing here. Maybe just omit this part?
There was a problem hiding this comment.
we could also just deprecate things and mention that visibility will be reduced in the next release. That way we can avoid breaking the API
There was a problem hiding this comment.
Maybe just omit this part?
Agree, i just wanted to emphasize that there is no known implementation who would be using it. but it may be possible that some java based catalog have started implementing the server side planning with these models but even for them it would be like a no-op, as the client would not be relying on the field they populated nor it will show up in the rest response (or post-serde)
Thinking more about marking this @deprecated, since we are still using its gonna start generating warnings, which IMHO is fine
please let me know your thoughts considering above
There was a problem hiding this comment.
I think having the warning is fine for one release. After one release we'll reduce visibility and remove the deprecation - which will also remove the warning again
|
|
||
| protected Map<Integer, PartitionSpec> specsById() { | ||
| /** | ||
| * @deprecated : visibility will be reduced in 1.12.0. |
There was a problem hiding this comment.
| * @deprecated : visibility will be reduced in 1.12.0. | |
| * @deprecated since 1.11.0, visibility will be reduced in 1.12.0. |
minor: we typically use that kind of pattern when deprecating
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM with a comment around the deprecation msg
The `specsById` map carried on `BaseScanTaskResponse` and its builder is parser context, not part of the REST spec for scan-planning response models. It was deprecated in 1.11.0 (apache#14485) with a note that visibility would be reduced in 1.12.0. Demote `specsById()` getter and `withSpecsById()` setter (plus the builder's `deleteFiles()` getter) from public to protected so the parsers and subclasses can still use them while keeping the field out of the public API. To preserve cross-package construction paths used by `CatalogHandlers`, `RESTServerCatalogAdapter`, and tests in `org.apache.iceberg.rest`: - Add `builder(Map<Integer, PartitionSpec> specsById)` factory on each of `PlanTableScanResponse`, `FetchPlanningResultResponse`, and `FetchScanTasksResponse` for initial server-side construction. - Add `toBuilder()` on each response for copy-with-modification patterns (used when adapters rebuild a response to inject credentials or rewrite status). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 participants
About the change
specs-by-id is not part of the rest spec for the remote scan planning api responses, this was mostly there to aid serialization, for Deserilization we used parser context (orthogonally), following the prinicipal that Response model should only have public APIs for things defined in spec, removing the visibility from public to protected to help avoid confusion that spec-by-id is part of spec.
We can still make sure post deserialization we have specsById as part of Parser context, this change is mainly motivated by rest-spec and existing response models.
Testing
Existing test pass and the compilation succeeds !