mbutrovich commented on PR #11732: URL: https://github.com/apache/arrow-rs/pull/11732#issuecomment-6369633826
> What do you think about dropping the additional thresholds from the latest commit and merging the simpler version first? We could then add the new NullBufferBuilder method in a follow-up before exploring the word-at-a-time optimization. I don't think we need to rush this and would rather get the design right. I'm also not sure the per-bit path is the simpler version, with one cutoff or two. Each cutoff is tuned to the benchmark inputs I listed [above](https://github.com/apache/arrow-rs/pull/11732#issuecomment-6325226430), and dropping the second one brings back the 0.005 regression from [my runs](https://github.com/apache/arrow-rs/pull/11732#discussion_r4338277053). I'd rather not merge a cutoff that the next PR removes. If you'd like to land something first, what about the version with no cutoff at all, where `copy_rows_by_filter_from` always calls `filter.filter()`? That's the "kernel" column in my table, and it beat `main` in nine of ten cases. As I read it, the gain comes from skipping the [`concat`](https://github.com/apache/arrow-rs/blob/df4dc3ede59bd43b5f71847231212381d068bb72/arrow-select/src/coalesce/generic.rs#L94-L103) in `GenericInProgressArray::finish`. The fused loop would then replace only that one call, so nothing merged along the way gets deleted. The order would be this PR, then `NullBufferBuilder::append_word`, then the fused loop. What do you think? -- 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]
