zenfenan commented on PR #3658: URL: https://github.com/apache/iceberg-python/pull/3658#issuecomment-5060806650
> Thank you for addressing the feedback. This looks good to me! > > One thought: a simpler alternative might be to convert `fetch_manifest_entry` into a generator instead of adding entry_filter. That way _open_manifest keeps its existing comprehension filter and the memory win comes for free, with no new parameter. It would be a wider change though, so not sure what maintainers would prefer here. Thanks! Good idea. A generator would give the memory win without a new parameter. Two things make me lean toward keeping the current approach here, though: it changes the return-type contract (callers doing `len()`/indexing including some existing tests would need `list()` wrapping), and it changes resource semantics since the Avro file handle would stay open for the lifetime of iteration rather than being read and closed up front. Happy to explore the generator conversion as a separate PR if maintainers think the broader change is worth 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]
