Revanth14 commented on PR #1213: URL: https://github.com/apache/iceberg-go/pull/1213#issuecomment-4720362200
> Nice way to make this reviewable as Go. Calling out the open design issues inline made it much easier to engage with the actual choices, and keeping local planning as the default feels right. > > I’d hold before merge though. This PR is setting the contract that Phases 5/6 and the scan-task decoder build on, so the wire types and public seam are the load-bearing part here. > > Main issue: the response envelopes don’t match the spec. The json tag is `plan-status`, but the spec field is `status`, so a conforming server response unmarshals into zero-value status. Also, `PlanTableScanResponse` / `FetchPlanningResultResponse` / `FetchScanTasksResponse` collapse the spec union into status-only structs, which drops the failed error payload, `storage-credentials`, and `plan-tasks`. I don’t think that shape should be deferred to the decoder PR — this is the contract the follow-ups will inherit. > > One smaller behavior issue: `WithScanPlanningMode` panics when the option is constructed, not when it’s applied. That breaks the no-behavior-change claim as soon as it appears in an option slice. Easy fix: move the panic into the returned closure. > > Things I’d settle here before follow-ups build on it: > > * fix the `status` json tag and give the three response envelopes the real spec shape > * make `WithScanPlanningMode` panic on apply, not construction > * settle the `ScanPlanningResult.IO` carrier; exporting a live `FSysF` will become stable once impl lands > * either add the server / `ScanPlanningRequired` mode, or drop the mention > * fix the `CaseSensitive` default mismatch: spec true vs Go zero-value false > > Once the contract is settled, happy to re-review the dependent phases against it. Thanks for the detailed review. I pushed a follow-up commit addressing these. Summary of changes: - Changed the planning response discriminator to `status` and renamed the field to `Status`. - Reshaped the planning responses around the spec union using flat envelopes with `PlanningError`, `ScanTasks`, and `StorageCredentials`. - Added `CompletedPlanningResult` and made `WaitForPlan` return that instead of the generic poll response. - Added `plan-tasks`, `file-scan-tasks`, and `delete-files` to the relevant response envelopes. - Moved `WithScanPlanningMode`’s unimplemented panic into the returned option closure. - Replaced the provisional live `FSysF` carrier with an optional `table.PlanIO` loader. - Documented the `scan-planning-mode=server` mapping as a REST table-config directive, distinct from the user-facing `local` / `remote` / `auto` scan option. - Documented that `CaseSensitive` is resolved from `Scan`, where the default is initialized to true. I also added the missing request fields from the REST spec (`min-rows-requested`, `stats-fields`) and made `StorageCredential` an exported REST type since completed planning responses now use it. -- 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]
