andygrove commented on code in PR #4885:
URL: https://github.com/apache/datafusion-comet/pull/4885#discussion_r3692632741
##########
native/spark-expr/src/string_funcs/base64.rs:
##########
@@ -98,12 +110,43 @@ fn chunk_into_lines(encoded: String) -> String {
out.push_str(&encoded[offset..end]);
offset = end;
}
+}
+
+fn encode_array<O: OffsetSizeTrait>(array: &GenericBinaryArray<O>, chunk:
bool) -> StringArray {
+ // Right-size the value buffer in O(1) from the input's total byte count.
When chunking,
+ // add the CRLF slack the encoded output will contain so the long-input
path does not grow.
+ let total_bytes = array.value_data().len();
+ let encoded_total = base64_encoded_len(total_bytes);
+ let data_capacity = if chunk {
+ chunked_len(encoded_total)
Review Comment:
Correct on all counts — the estimate was not a bound at all for arrays of
short values, and the "sizes the value buffer up front" claim only held for the
few-large-row shape. Fixed in b1acaefbc.
Since rows pad independently, the encoded total is `sum ceil(len_i / 3) *
4`, which genuinely cannot be recovered from the summed input bytes. But `sum
ceil(x_i) <= ceil(sum x_i) + (N - 1)`, so adding `4 * (N - 1)` turns it into a
true upper bound while staying O(1) — no pass over the offsets, so this does
not walk back the earlier round. It over-reserves by at most 4 bytes per row,
which only bites for arrays of very short values, where the buffer is small in
absolute terms anyway. The chunked case then applies `chunked_len` to that
bound, which is still an upper bound because `sum floor((enc_i - 1) / 76) <=
floor((sum enc_i - 1) / 76)`.
Extracted into `encoded_capacity` with the reasoning in a doc comment, and
pinned by `encoded_capacity_is_an_upper_bound`, which compares the estimate
against the actual encoded total for eight shapes — including the `vec![1; 64]`
worst case that motivated this, all-empty, exactly-one-line, wraps-once, mixed,
and few-large-rows — under both chunk settings. `encode_array_many_tiny_rows`
covers output correctness for that shape separately.
On encoding straight into the builder's value buffer: that turned out to be
possible and is now done (see the other thread), which is also why this
estimate is back to being a pure hint rather than something the hot path
depends on.
--
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]
