mbutrovich commented on code in PR #4885:
URL: https://github.com/apache/datafusion-comet/pull/4885#discussion_r3590962377
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
@@ -62,11 +64,41 @@ pub fn spark_base64(args: &[ColumnarValue]) ->
Result<ColumnarValue, DataFusionE
}
}
+const LINE_LEN: usize = 76;
Review Comment:
`LINE_LEN` itself is single-sourced at module scope (`base64.rs:67`), so the
threshold value cannot drift. What lives in two places is the decision to wrap:
the array path inlines `chunk && encoded.len() > LINE_LEN` (`base64.rs:93`)
while the scalar path relies on `chunk_into_lines`'s internal `encoded.len() <=
LINE_LEN` guard (`base64.rs:118`). The two are consistent today (both leave a
76-char string unwrapped), but the comparison lives in two places and a future
edit to one can silently diverge from the other.
Suggested change: have the array path call a single shared helper that owns
the threshold, for example `maybe_chunk(&encoded, chunk, &mut wrapped)` that
returns which buffer to append, so `chunk_into_lines` and the array loop cannot
drift. This also removes the near-duplicate wrapper `chunk_into_lines` /
`chunk_into_lines_buf` split.
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
@@ -62,11 +64,41 @@ pub fn spark_base64(args: &[ColumnarValue]) ->
Result<ColumnarValue, DataFusionE
}
}
+const LINE_LEN: usize = 76;
+
+/// Length of the padded base64 encoding of `n` input bytes.
+fn base64_encoded_len(n: usize) -> usize {
+ n.div_ceil(3) * 4
+}
+
fn encode_array<O: OffsetSizeTrait>(array: &GenericBinaryArray<O>, chunk:
bool) -> StringArray {
- array
- .iter()
- .map(|value| value.map(|bytes| encode(bytes, chunk)))
- .collect()
+ // Encode into a builder, reusing scratch buffers across rows. This avoids
a
+ // fresh per-row heap allocation for each element's base64 string (and,
when
+ // chunking, its line-wrapped copy) that the previous per-element
+ // `encode()` + `collect()` incurred, and sizes the value buffer up front.
+ let data_capacity: usize = (0..array.len())
+ .filter(|&i| array.is_valid(i))
+ .map(|i| base64_encoded_len(array.value(i).len()))
+ .sum();
Review Comment:
`base64.rs:79-82` iterates the whole array once to sum encoded lengths, then
the main loop (`base64.rs:86-100`) iterates again. Each `array.value(i)` call
in the capacity pass reconstructs a slice. For the chunked long case the sum
also omits the CRLF bytes, so the value buffer still reallocates during
wrapping, which is exactly the "long, chunked" benchmark row.
Suggested change: get the total input byte count in O(1) from
`array.value_data().len()` and derive a single upper bound, for example
`base64_encoded_len(total_bytes)` plus, when `chunk`, a CRLF slack term. This
drops the extra per-row pass and stops under-sizing the wrapped output. If you
keep the per-row exact sum, at minimum add the separator bytes into
`data_capacity` when `chunk` is true.
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
Review Comment:
`native/spark-expr/src/string_funcs/base64.rs:74-102` is the code this PR
rewrites, yet both existing tests (`base64.rs:147` and `base64.rs:157`) only
call `encode()`, which is the scalar path. `encode()` is unchanged by this PR.
Nothing exercises the new builder loop, the null branch (`base64.rs:87-89`),
the capacity pass, the `LargeBinary` (`i64`) generic instantiation, or the
array-level wrap condition. The differential fuzz gate covers this at runtime,
but a regression here would not be caught by `cargo test`.
Suggested change: add a test that builds a `BinaryArray` and a
`LargeBinaryArray` mixing nulls, empty values, a value that encodes to exactly
76 chars, and a value that wraps twice, then asserts the resulting
`StringArray` element-for-element (including a null in the middle). Reuse the
`[b'a'; 57]` / `[b'c'; 120]` fixtures already in the chunked test so the
expected wrapping is known.
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
@@ -83,12 +115,19 @@ fn encode(bytes: &[u8], chunk: bool) -> String {
/// offsets and character offsets coincide. Takes the string by value so the
common short-input
/// case (no wrapping needed) returns it without a second allocation.
fn chunk_into_lines(encoded: String) -> String {
- const LINE_LEN: usize = 76;
if encoded.len() <= LINE_LEN {
return encoded;
}
+ let mut out = String::new();
+ chunk_into_lines_buf(&encoded, &mut out);
+ out
+}
+
+/// Append the CRLF-wrapped form of `encoded` (lines of at most 76 characters)
into `out`.
Review Comment:
`base64.rs` doc comment on `chunk_into_lines_buf` says "The caller is
expected to have cleared `out`; only used when `encoded.len() > LINE_LEN`." The
function itself is correct for any input (it reserves and appends), so the
"only used when" clause is a usage note, not a precondition, and the "expected
to have cleared" wording reads like an unchecked invariant. State why the
buffer is reused (to avoid per-row allocation) rather than describing current
call sites.
Suggested change: shorten to "Appends the CRLF-wrapped form of `encoded` to
`out`. `out` is reused across rows to avoid a per-row allocation, so the caller
clears it before each call."
--
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]
