andygrove commented on PR #23458:
URL: https://github.com/apache/datafusion/pull/23458#issuecomment-4939331683
> Nice improvement overall — the null-free fast path clearly removes a lot
of `Option` and null bookkeeping overhead, and the benchmark wins in the PR
description are compelling.
>
> One additional optimization opportunity: the current fallback path handles
all null-containing cases together, so it still pays for `x.is_valid(i)` and,
in the NaN case, `y.is_valid(i)` checks inside a single generic loop. I think
this can be tightened further by splitting the fallback into separate cases for:
>
> * `(None, Some(_))`
> * `(Some(_), None)`
> * `(Some(_), Some(_))`
>
> That should reduce branching and avoid unnecessary validity checks for
partially-null inputs while preserving the same semantics.
>
> Concretely, I’d expect something along these lines in `nanvl_impl`:
>
> ```rust
> match (x.nulls(), y.nulls()) {
> (None, None) => { /* current fast path */ }
> (None, Some(_)) => { /* only y validity matters when x_value.is_nan()
*/ }
> (Some(_), None) => { /* only x validity matters; y validity checks
disappear */ }
> (Some(_), Some(_)) => { /* current generic null-aware path */ }
> }
> ```
>
> Also, a couple of smaller follow-ups:
>
> * the PR description says “raw f64 value slices”, but the implementation
is now generic across `Float16/32/64`
> * it would be useful to add benchmarks for partially-null inputs too,
since the current numbers mostly validate the no-null fast path
I tried out the main suggestion here and saw worse performance. Curious to
know if you can reproduce this?
--
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]
