Revanth14 commented on PR #1213:
URL: https://github.com/apache/iceberg-go/pull/1213#issuecomment-4763600910
> Almost there, things I'd like to settle in this PR before the follow-ups
build on it:
>
> * The `Marshal` request-filter wire format above.
> * Whether exported panic-bodied functions should sit in `main` at all — a
release cut before implementation lands ships them as crash-on-call API.
Returning `fmt.Errorf("not yet implemented: %w", ErrNotImplemented)`,
`internal/` placement, or a build tag all avoid that.
> * One concrete `Catalog -> Table -> Scan` wiring option, even as a
committed comment — it's the one architectural piece everything downstream
needs and it's currently only prose.
> * The `CaseSensitive bool` default trap and the
`WaitForPlanOptions.Timeout`-vs-`ctx` duplication, since both shape the public
types.
>
> The rest of my notes are inline and most are "let's make this explicit"
rather than blockers. I'd defer the dependent delegation/decoder PRs until
these are settled. Once they are, happy to take another pass and approve
Thanks for the detailed pass. I went through the inline notes and addressed
the contract-shaping items in this PR.
Quick summary:
- Kept `MarshalExpressionJSON` Java/reference-compatible after checking
`PlanTableScanRequestParser -> ExpressionParser`; marshal emits bare booleans,
unmarshal stays permissive.
- Removed crash-on-call exported stubs: methods/codecs now return
`ErrNotImplemented`-wrapped errors, `Supports*` returns false, and
`WithScanPlanningMode` is a real option setter.
- Added concrete `Catalog -> Table.planner -> Scan.planner` wiring without
changing `table.New`'s signature.
- Changed `CaseSensitive` / `UseSnapshotSchema` to `*bool`, and dropped
`WaitForPlanOptions.Timeout` in favor of context deadlines.
- Also handled the inline envelope items: typed REST task placeholders,
plan-id/cancelled validation, request header fields, reused REST error payload,
and deferred incremental fields.
--
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]
