C-Loftus commented on PR #1426: URL: https://github.com/apache/iceberg-go/pull/1426#issuecomment-4958042729
Thanks for the review @zeroshade ; at your convenience would you be able to give your thoughts on Andrei's comment here https://github.com/apache/iceberg-go/pull/1426#discussion_r3558214926 ? Namely: > @zeroshade — you rejected widening the visitor interface on https://github.com/apache/iceberg-go/issues/1230; does the ctx-carries-required-props shape sit right with you, or would you rather see properties passed explicitly? This decision drives most of the rest of the PR, so I'd like to settle it first. From my understanding, prior discussions in #1230 Andrei said that: > The read path threads table properties through context (arrow_scanner.go does ctx = internal.WithTableProperties(ctx, as.metadata.Properties()) and parquet_files.go reads it back on the other side). This is the write-side mirror of that same problem, so I'd rather reuse that pattern than invent a second mechanism. I think there is a good argument to pass the data to the `Visit` call as you say here https://github.com/apache/iceberg-go/pull/1426#discussion_r3566791333 but just wanted to be aligned on if I should change the current approach of passing a `ctx`. No worries if approaches need to change, I am happy to make edits here! Thanks! -- 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]
