technicolorbeat commented on PR #1749: URL: https://github.com/apache/iceberg-go/pull/1749#issuecomment-5244442075
> Thanks — implementation is clean and the test is solid. Two things before this can move forward: > > 1. +1 to the open question from `alessandro-nori` on per-request vs. catalog-level configuration. Java configures `snapshot-loading-mode` once at catalog initialization, and this repo followed a similar context/config pattern for `pageSize`. I'd like to hear your take before we commit to the `LoadTableWithSnapshotMode` public API shape — a catalog property (possibly alongside this method) may serve the common case better. > 2. A few smaller points inline: query-string merging in `do()`, Go doc-comment convention on the new exported identifiers, and two cheap test additions. > > CI is green; happy to re-review once the API-shape question settles. > > _This review was drafted with an AI-assisted tool and may contain mistakes; an Apache Iceberg Go maintainer has reviewed and confirmed the submission. See the [contributing docs](https://github.com/apache/iceberg-go/blob/main/CONTRIBUTING.md) for what the project considers a maintainer review._ thanks for taking a look! thinking about this more, I agree the catalog-level implementation is a better than a per-request implementation since most of our users who want refs want it for all loads on that catalog and this is more ergonomic in that sense. I'll update this and address docu / test comments 🙏 -- 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]
