xixipi-lining commented on PR #1162: URL: https://github.com/apache/iceberg-go/pull/1162#issuecomment-4628758033
Hi @tanmayrauth , thanks for working on this fix! While this approach resolves the issue, I have an architectural concern regarding writerFactory. Storing tableProps on the factory just to pass them down into LoadLocationProvider for each partition adds extra state to it, which we should try to avoid to keep its responsibilities focused. If we look at the [Java implementation of LocationProvider](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/io/LocationProvider.java), it actually provides a partition-aware method: newDataLocation(PartitionSpec spec, StructLike partitionData, String filename) Long term, evolving our LocationProvider to be partition-aware like Java's would cleanly remove the need for writerFactory to manage tableProps or rebuild providers per partition. What are your thoughts on taking this direction? -- 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]
