nssalian commented on issue #2044: URL: https://github.com/apache/iceberg-go/issues/2044#issuecomment-6748966556
+1 on the approach, and on closing this once #2376 lands with the Substrait item left to #2062. I checked the plan against `main` (7a4fc6c): - **Surface.** #2376 exports only `Option`, `WithWarehouse`, `WithPrefix`, `WithProperties`, `NewHandler` and `NewServer`, keeps the wire types unexported and doesn't touch `catalog/rest`, so the v1 constraint holds. - **Split with the existing tests.** Keeping the wire-level `rest_test.go` cases (like `TestRenameTable404UntypedFallsBackToNoSuchTable`) and `planfake` separate from `resttest` makes sense to me. - **Step 1 of the follow-up.** Since #2350, the REST client reports commit 400 and 404 responses as `table.ErrCommitRejected`, and `resttest` returns 400 for a requirement that can't be fixed by a refresh. So the #2100 terminal case can assert `ErrCommitRejected` against `resttest`, not only "not `ErrCommitFailed`". #2376 needs a rebase past #2350 for that. - **Substrait.** It still comes in through `catalog/rest` → `table` → `table/substrait`, and #2062 tracks it, so nothing is lost by closing this. -- 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]
