sunchao commented on code in PR #7201:
URL: https://github.com/apache/datafusion-comet/pull/7201#discussion_r4384891530
##########
native/shuffle/benches/row_columnar.rs:
##########
@@ -374,6 +576,84 @@ fn benchmark_map_conversion(c: &mut Criterion) {
&schema,
&rows,
1,
+ 1.0,
+ );
+ }
+ }
+
+ group.finish();
+}
+
+/// Utf8 columns, plain and dictionary-encoded.
+///
+/// Strings are read from the variable-length region through an offset+size
pointer, unlike the
+/// fixed-width values of the cases above, and `prefer_dictionary_ratio` picks
between two
+/// builders for them:
+///
+/// - `plain`: ratio 1.0, so every value is appended to a `StringBuilder`.
+/// - `dictionary`: ratio 10.0, Comet's default, over low-cardinality values
(16 distinct per
+/// column), so the column stays `Dictionary(Int32, Utf8)`.
+/// - `dictionary_fallback`: ratio 10.0 over the plain case's distinct values,
so the column is
+/// built as a dictionary and then cast back to plain. This is what a
high-cardinality string
+/// column costs under the default ratio.
+///
+/// Every row is built separately, with values that vary in length so the
8-byte padding of the
+/// variable-length region varies too. Each fixture is converted and read back
once, and checked
+/// against the values it was built from, before it is timed.
+fn benchmark_string_conversion(c: &mut Criterion) {
+ let mut group = c.benchmark_group("string_conversion");
+ const NUM_COLS: usize = 10;
+
+ for (label, low_cardinality, prefer_dictionary_ratio) in [
+ ("plain", false, 1.0),
+ ("dictionary", true, 10.0),
Review Comment:
[P2] The `plain` and `dictionary` cases change the string data as well as
the encoding policy. Their timings therefore cannot establish the reported 54%
saving from retaining a dictionary. Add a ratio-`1.0` case over the same
`category_*` rows and assert `Utf8` output, or describe the published numbers
as timings for two different workloads.
Evidence: Across 10,000 rows and ten columns, `plain` contains 2,288,880
UTF-8 payload bytes and 10,000 distinct values per column. `dictionary`
contains 1,037,500 bytes and 16 distinct values per column. In a disposable
control holding the ratio at `1.0`, the converter wrote 78,393 compressed bytes
for the existing plain input and 125,787 for the low-cardinality input.
Changing the fixture alone changes the work inside the timed loop.
##########
native/shuffle/benches/row_columnar.rs:
##########
@@ -374,6 +576,84 @@ fn benchmark_map_conversion(c: &mut Criterion) {
&schema,
&rows,
1,
+ 1.0,
+ );
+ }
+ }
+
+ group.finish();
+}
+
+/// Utf8 columns, plain and dictionary-encoded.
+///
+/// Strings are read from the variable-length region through an offset+size
pointer, unlike the
+/// fixed-width values of the cases above, and `prefer_dictionary_ratio` picks
between two
+/// builders for them:
+///
+/// - `plain`: ratio 1.0, so every value is appended to a `StringBuilder`.
+/// - `dictionary`: ratio 10.0, Comet's default, over low-cardinality values
(16 distinct per
+/// column), so the column stays `Dictionary(Int32, Utf8)`.
+/// - `dictionary_fallback`: ratio 10.0 over the plain case's distinct values,
so the column is
+/// built as a dictionary and then cast back to plain. This is what a
high-cardinality string
+/// column costs under the default ratio.
+///
+/// Every row is built separately, with values that vary in length so the
8-byte padding of the
+/// variable-length region varies too. Each fixture is converted and read back
once, and checked
+/// against the values it was built from, before it is timed.
+fn benchmark_string_conversion(c: &mut Criterion) {
+ let mut group = c.benchmark_group("string_conversion");
+ const NUM_COLS: usize = 10;
+
+ for (label, low_cardinality, prefer_dictionary_ratio) in [
+ ("plain", false, 1.0),
+ ("dictionary", true, 10.0),
+ ("dictionary_fallback", false, 10.0),
+ ] {
+ for num_rows in [1000, 10000] {
+ let schema = vec![ArrowDataType::Utf8; NUM_COLS];
+ let values: Vec<Vec<String>> = (0..num_rows)
+ .map(|row| {
+ (0..NUM_COLS)
+ .map(|col| {
+ if low_cardinality {
+ format!("category_{}", (row + col) % 16)
+ } else {
+ format!("value {} of column {col}", row * 7919
% 100_003)
+ }
+ })
+ .collect()
+ })
+ .collect();
+ let rows: Vec<Vec<u8>> = values.iter().map(|row|
build_string_row(row)).collect();
+
+ let batch = convert_and_read_back(&schema, &rows,
prefer_dictionary_ratio);
Review Comment:
[P2] Listing one existing benchmark now writes and decodes all 16 new
fixtures. This call and its map and nullable counterparts run before
`bench_with_input` applies Criterion’s filter. A failure in a skipped fixture
can therefore abort discovery or an unrelated benchmark. Initialize and
validate each fixture once when Criterion invokes its selected callback,
outside `b.iter`.
Evidence: `row_columnar --list --exact primitive_columns/cols_100/rows_1000`
printed the same ID on both revisions but created zero temporary files on the
merge base and 16 on the head. With a nonexistent `TMPDIR`, the base exited 0
and the head exited 101 at `tempfile().unwrap()`. Running `--test` for that
same ID created one file on the base and 17 on the head.
--
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]
