andygrove opened a new issue, #23674:
URL: https://github.com/apache/datafusion/issues/23674
### Is your feature request related to a problem or challenge?
Follow-on from review feedback on #23473, which rewrote `hex_encode_bytes`
(`datafusion/spark/src/function/math/hex.rs`) to write hex digits directly into
a single value buffer. Two further optimizations were suggested during review
but left out of that PR to keep it focused:
1. **Reuse the input `NullBuffer` instead of rebuilding one.** The current
implementation constructs a fresh `NullBufferBuilder` and calls
`append_null`/`append_non_null` per row. Since hex encoding is 1:1 with the
input rows (a null in maps to a null out), the output nulls are identical to
the input's. We could clone the input array's `NullBuffer` directly instead of
rebuilding it. This requires plumbing the input array (or its nulls) through to
`hex_encode_bytes`, which currently only receives an `Iterator<Item =
Option<T>>` and has no access to the underlying `NullBuffer`.
> we can probably avoid a null builder by just cloning the input nulls?
though i guess need to plumb the functions to pass those through
> — @Jefffrey,
https://github.com/apache/datafusion/pull/23473#discussion_r3577957866
2. **Special-case the "no nulls" path.** When the input `NullBuffer` is
`None`, there is no need to check each value for `Some`/`None`. We could split
into two loops — one tight loop for the no-nulls case and one for the has-nulls
case.
> Another optimization is to special case the "has nulls" path (aka if
the input NullBuffer is None, there is no need to check each value for `Some` /
`None` too -- instead we could have two loops
> — @alamb,
https://github.com/apache/datafusion/pull/23473#discussion_r3597744534
### Describe the solution you'd like
Refactor `hex_encode_bytes` in `datafusion/spark/src/function/math/hex.rs`
to:
- Accept the input array (or its `NullBuffer`) so the output nulls can be
cloned directly rather than rebuilt via `NullBufferBuilder`.
- Take separate code paths for the no-nulls and has-nulls cases, avoiding a
per-value `Option` check when the input has no nulls.
Validate with the existing `datafusion/spark/benches/hex.rs` benchmark
against `main`.
### Describe alternatives you've considered
Leaving the implementation as-is; it already avoids the per-row copy that
#23473 targeted. These are incremental improvements on top of that.
### Additional context
Both suggestions came from review of #23473.
--
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]
