alamb commented on code in PR #10512:
URL: https://github.com/apache/arrow-rs/pull/10512#discussion_r3703309989
##########
parquet/benches/arrow_writer.rs:
##########
@@ -651,5 +676,55 @@ fn bench_all_writers(c: &mut Criterion) {
}
}
-criterion_group!(benches, bench_all_writers);
+/// Large-value BYTE_ARRAY columns under `DELTA_BYTE_ARRAY`, which the
+/// property matrix above never exercises (it only varies writer version,
+/// compression, bloom filters, and CDC — all on the default encoding).
+///
+/// Values are sized so a single value exceeds the default 1 MiB data page
+/// limit, putting the writer in the byte-budget sub-batching regime of
+/// `write_batch_internal`. That budget measures raw payload bytes, so under
+/// `DELTA_BYTE_ARRAY` it sub-batches by the values' *pre-dedup* size however
+/// well they compress; benchmarking against `PLAIN` on the same data bounds
+/// what an encoded-size-aware budget could recover. The shared-prefix and
+/// distinct batches bracket the encoding's best and worst case.
+fn bench_delta_byte_array_writers(c: &mut Criterion) {
+ // 128 rows × 2 MiB: the same total bytes as `large_string_non_null`
+ // above, but with each value alone exceeding the default 1 MiB page
Review Comment:
I think these comments will go stale as soon as this PR is merged (this are
basically comments for the PR, not the code)
##########
parquet/benches/arrow_writer.rs:
##########
@@ -651,5 +676,55 @@ fn bench_all_writers(c: &mut Criterion) {
}
}
-criterion_group!(benches, bench_all_writers);
+/// Large-value BYTE_ARRAY columns under `DELTA_BYTE_ARRAY`, which the
+/// property matrix above never exercises (it only varies writer version,
+/// compression, bloom filters, and CDC — all on the default encoding).
+///
+/// Values are sized so a single value exceeds the default 1 MiB data page
+/// limit, putting the writer in the byte-budget sub-batching regime of
Review Comment:
this is talking about the internal implementation of `write_batch_internal`
which is not in this meethog -- I think it would be clearer if it talked about
the properties of the data instead.
For example, perhaps give some examples of the data strings being compressed
(`xxxx.....001`, etc) and note that the shared prefix case is the case that
DLBA is designed to handle well
##########
parquet/benches/arrow_writer.rs:
##########
@@ -651,5 +676,55 @@ fn bench_all_writers(c: &mut Criterion) {
}
}
-criterion_group!(benches, bench_all_writers);
+/// Large-value BYTE_ARRAY columns under `DELTA_BYTE_ARRAY`, which the
+/// property matrix above never exercises (it only varies writer version,
Review Comment:
referring to the current state of the above matrix may get stale eventually
--
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]
