Revanth14 commented on PR #1213:
URL: https://github.com/apache/iceberg-go/pull/1213#issuecomment-4795574670
> Really like this as a way to settle the seam before the implementation
phases land — the doc comments carry the open questions well, and the wiring
decisions (Catalog → Table → Scan via a `ScanPlanner` assertion with no `New`
signature change, the `local`/`remote`/`auto` option kept distinct from the
spec's `client`/`server` directive, committing named
`RESTFileScanTask`/`RESTDeleteFile` instead of `json.RawMessage`) are the right
calls. To be clear up front: I'm not treating the unimplemented stubs as
findings — reviewing the surface is the whole point of the PR.
>
> I'd hold this before merging though, since the rest of the 8-phase stack
builds directly on this surface and a couple of these shapes get expensive to
change once the follow-ups depend on them.
>
> The one I'd most want settled is `PlanIO` and how it reaches the reader.
`ScanPlanningResult` returns `{Tasks, IO}`, but `(*Scan).PlanFiles` only
returns tasks, so the plan-scoped FileIO that OQ1 says is "resolved" has no
path to `ReadTasks` today — getting there means either a breaking signature
change or stashing the IO on `Scan`, which is racy under concurrent scans. That
delivery mechanism decides whether the surface is shaped right, so I'd pin it
down here. The related call is lifecycle: plan-scoped creds are time-bounded
and Java closes the plan IO on GC, so I'd lean toward keeping `PlanIO` an
interface and adding `Close()` rather than collapsing it to `FSysF`.
>
> The other is `PlanningError = errorResponse`. Aliasing the internal
HTTP-error type into the public failed-result model leaks
`Error()`/`Unwrap()`/the unexported `wrapping` field, renders as
`errorResponse` in godoc, and can't be constructed by callers. I'd make it a
plain exported struct mirroring `ErrorModel` (`stack` included — it is in the
spec). And there's one real correctness gap in the only live code: the
`UnmarshalJSON` switch silently accepts `failed` and unknown statuses as a
successful empty scan, with a latent nil-deref on `*PlanID`.
>
> Things I'd like to settle in this PR before the follow-ups build on it:
>
> * `PlanIO` delivery path through `(*Scan).PlanFiles` → `ReadTasks`, plus
the interface-vs-`FSysF` and `Close()` lifecycle call
> * `PlanningError` as a real exported struct, not an alias for
`errorResponse`
> * The `failed`/default arm in `PlanTableScanResponse.UnmarshalJSON` (+
companion tests)
> * Missing `data-access` path on `FetchPlanningResult`, so async polls can
still vend storage-credentials
> * Narrowing `ScanPlanningRequest.Metadata` to the smaller interface you
already flag in the doc comment
> * The `expression_json.go` doc comment —
`{"type":"true"}`/`{"type":"false"}` isn't a real encoding; it should say bare
booleans + `{"type":"literal","value":...}`
>
> A few smaller things I left out of inline comments but are worth a sweep:
the `ScanPlanningMode` godoc could state the `local`↔`client` /
`remote`↔`server` mapping explicitly; `Table.Refresh()` doesn't copy `planner`;
the new test file uses stdlib `t.Fatalf`/`errors.Is` instead of the package's
testify `require`/`assert` and skips `t.Parallel()`; the happy-path test
decodes into an empty `RESTFileScanTask{}` so its `len == 1` assertion is
vacuous (worth a TODO for when the decoder lands); and a `// TODO(Phase 6)` to
resolve `planningMode` from table properties at both `Scan()` sites.
>
> The correctness items are small; most of the list is about locking the
surface in the right place. Once those are settled, happy to take another pass
and approve.
Thanks, addressed the requested changes.
Settled the `PlanIO` path by keeping it as an interface, adding `Close()`,
and making `Scan` own the returned plan IO for the `PlanFiles -> ReadTasks`
sequence. `PlanningError` is now a real exported struct.
`PlanTableScanResponse.UnmarshalJSON` now validates failed/unknown/cancelled
statuses with tests. `FetchPlanningResult` / `WaitForPlan` can carry access
delegation, `ScanPlanningRequest.Metadata` is narrowed, and the expression JSON
docs now call out the Java-compatible forms.
Also done the smaller items: planning-mode docs, `Table.Refresh` planner
copy, Phase 6 TODOs, and test style/parallelization.
--
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]
