andygrove commented on code in PR #4885:
URL: https://github.com/apache/datafusion-comet/pull/4885#discussion_r3604020866
##########
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`.
+/// The caller is expected to have cleared `out`; only used when
`encoded.len() > LINE_LEN`.
+fn chunk_into_lines_buf(encoded: &str, out: &mut String) {
let separators = (encoded.len() - 1) / LINE_LEN;
- let mut out = String::with_capacity(encoded.len() + separators * 2);
+ out.reserve(encoded.len() + separators * 2);
Review Comment:
The scratch buffer is now reused across rows (see \`encode_array\` — one
\`buf\` outside the loop), so we only pay for a resize on rows that outgrow the
previous row.
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
Review Comment:
Added `encode_array_binary_chunked` (BinaryArray with mixed nulls, empty
value, exactly-76 boundary, wrap-once, wrap-twice) and
`encode_array_large_binary_unchunked` covering the LargeBinaryArray i64 offset
instantiation.
##########
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:
Replaced the O(n) pre-pass with an O(1) estimate:
`base64_encoded_len(array.value_data().len())`, plus `chunked_len(...)` slack
when `chunk` is true. The wrapped-output long-input path no longer
under-reserves.
##########
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:
Consolidated into a single `encode_into(bytes, chunk, out)` helper. The
threshold decision lives in one place; the scalar `encode` and array
`encode_array` both call it. Removed the near-duplicate
`chunk_into_lines`/`chunk_into_lines_buf` split.
##########
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:
Rewrote the doc on the encoding helper: `out is reused across rows to avoid
per-row heap allocations; the caller clears it before each call` (no more `only
used when` clause that reads like a precondition).
--
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]
