Revanth14 commented on PR #1213:
URL: https://github.com/apache/iceberg-go/pull/1213#issuecomment-4747052785
> The contract pieces from round one landed cleanly: status discriminator
and spec union shape, typed `CompletedPlanningResult` from `WaitForPlan`,
shared `ScanTasks` envelope, `WithScanPlanningMode` panicking on apply, and the
`PlanIO` loader. With dillitz’s spec-side approval too, nothing here blocks.
>
> This is mostly pre-implementation cleanup before the next phases build on
this surface.
>
> What I’d still fold in:
>
> * `expression_json.go`: document the format decision in the doc comment.
If we emit Java’s bare `true` / `false`, say that, and say
`UnmarshalExpressionJSON` accepts both bare booleans and the spec
`{"type":"true"}` form. This one matters most because the Phase 1 golden
fixtures will lock in whichever format we choose.
> * `plan-id` on completed `PlanTableScan` responses: the spec requires it,
and without it there’s no handle for `CancelPlanning`. I’d make it required on
a dedicated completed response type, or validate it at decode time.
> * `cancelled` status: spec says it’s invalid as a `PlanTableScan` result,
so the client should treat it as an error, not a normal state.
> * receiver naming: new `*Catalog` methods use `c`, while the rest of the
package uses `r`; worth aligning before lint complains.
>
> None of this changes behavior today — it just keeps the contract clean
before the decoder and scanner-delegation PRs inherit it.
>
> Happy to approve once the expression format is documented and the
`plan-id` shape is firmed up.
Addressed the suggestions, happy to modify if need anything.
@laskoviymishka
--
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]
