Jeadie commented on PR #10236: URL: https://github.com/apache/arrow-rs/pull/10236#issuecomment-4997129873
> my main concern is we seem to be duplicating a lot of code because of this dyn iterator approach; `filter_list_primitive`, `filter_list_bytes` and `filter_nulls_ranges` are essentially versions of `filter_primitive`, `filter_bytes` and `FilterPredicate::filter_nulls` but specialized towards accepting a dyn range iter instead of a `FilterPredicate` > > i see why the approach with dyn iterator was taken, as: > > * it allows converting from index iterator to slice iterator > * it allows mapping to child offsets iterator > > but i do wonder if its feasible to try unify this into `FilterPredicate` and its strategy related code to see if we can avoid this duplication 🤔 I did consider adding a `IterationStrategy` variant, but a list child's range is inherently borrowed. We would need to either: 1. Add lifetimes to both `IterationStrategy<'a>` and `FilterPredicate<'a>` 2. Materialise the range and use `IterationStrategy::Slices(Vec<(usize, usize)>)`. This would revert much of the gains of this PR. ```rust IterationStrategy::Ranges(Box<dyn Fn() -> Box<dyn Iterator<Item=(usize,usize)>>>) ``` Saying this, I have made some updates to reduce code duplication in this area. -- 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]
