sunchao commented on code in PR #7106:
URL: https://github.com/apache/datafusion-comet/pull/7106#discussion_r4320381503
##########
native/spark-expr/src/conversion_funcs/cast.rs:
##########
@@ -874,6 +875,44 @@ fn cast_binary_to_string<O: OffsetSizeTrait>(
.downcast_ref::<GenericByteArray<GenericBinaryType<O>>>()
.unwrap();
+ // `new String(bytes, UTF_8)` leaves valid UTF-8 unchanged. Check the
whole value buffer once:
+ // when it is valid, reuse the input's buffers instead of copying every
value. Otherwise rows
+ // inside its valid prefix are copied without being checked again, and
only the rest, starting
+ // at the first invalid byte (which may sit under a null or outside a
slice), are decoded.
+ let mut valid_prefix = "";
+ if matches!(
+ spark_cast_options.binary_output_style,
+ None | Some(BinaryOutputStyle::Utf8)
+ ) {
+ let data = input.value_data();
Review Comment:
[P2] [P2] Avoid validating the entire backing buffer for every slice.
`BinaryArray::slice` retains the original value buffer, so `input.value_data()`
includes bytes outside the logical array. Casting consecutive slices now
rescans all those bytes on every invocation, whereas the previous decoder
processed only visible values. This affects existing binary-to-string casts
over sliced batches, including chunked aggregate output. For 262,144 valid
64-byte values emitted as 32 slices of 8,192 rows, conversion increased from
3.29 ms to 24.99 ms (7.59×), with identical output. Could we retain the
previous path for sliced arrays, or slice the values and rebase offsets before
validation? That preserves the whole-array improvement without making
conversion cost scale with unrelated backing data.
Evidence: Ran `CARGO_TARGET_DIR=/tmp/comet-pr7106-kernel/target cargo run
--release --offline --manifest-path /tmp/comet-pr7106-head-kernel/Cargo.toml --
--chunks-only`. The harness uses Arrow 59.3.0 and helpers verified
byte-for-byte against the supplied base and head. It builds a 16 MiB
BinaryArray of 262,144 valid 64-byte values, casts `parent.slice(i * 8192,
8192)` for i=0..31, and checks output equality. Median timings over seven
alternating groups were base=3.291 ms and head=24.987 ms. A separate 128-row
slice of an 8 MiB buffer measured 5.690→205.707 µs. Arrow 59.3.0
`GenericByteArray::slice` clones the full value buffer. DataFusion 55.2.0
`MaterializedAggregateOutput::next_batch` emits these shared-buffer slices via
`self.batch.slice(self.offset, length)`. Measurements isolate conversion cost
rather than whole-query runtime.
--
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]
