Copilot commented on code in PR #24328:
URL: https://github.com/apache/datafusion/pull/24328#discussion_r3776462682
##########
benchmarks/bench.sh:
##########
@@ -107,6 +107,9 @@ wide_schema: Small-projection queries on a wide
synthetic dataset (10
predicate_eval: Conjunctive (AND) filter-evaluation micro-benchmarks;
each subgroup is a different predicate pattern, to test how an
adaptive predicate-ordering system behaves across
them (see https://github.com/apache/datafusion/issues/11262)
(subgroups via BENCH_SUBGROUP: costsel, cost,
selectivity, cardinality, width, scale, neutral, correlation, drift)
+parquet_row_filter_skip: Per-RG fully-matched RowFilter skip on Parquet
(apache/datafusion#23696); clustered string key + low-selectivity
+ range filter + pushdown, so most row groups are
fully matched and the per-row RowFilter is skipped on them
+ (data generated inline by the suite's load SQL;
knobs: PRED_ROWS, RG_SIZE)
(toggle a system under test with its native
DATAFUSION_* env var; size data with PRED_ROWS, string width with PRED_FILL)
Review Comment:
This help/usage text line about toggling a system under test (and
`PRED_FILL`) belongs to `predicate_eval`, but after inserting the new
`parquet_row_filter_skip` entry it now appears as part of the new suite’s
description. That makes the bench list misleading.
Move the toggle line back under `predicate_eval` and keep the
`parquet_row_filter_skip` description limited to its own knobs (`PRED_ROWS`,
`RG_SIZE`).
##########
benchmarks/sql_benchmarks/parquet_row_filter_skip/load/clustered.sql:
##########
@@ -0,0 +1,39 @@
+-- Clustered Parquet dataset for the fully-matched RowFilter-skip benchmark.
+--
+-- `skey` is a fixed-width, zero-padded, monotonically increasing string, so
+-- each row group holds a disjoint, sorted range of keys. With
+-- `pushdown_filters=true`, a low-selectivity range predicate (see the query)
+-- leaves the first row group straddling and every later row group fully
+-- matched by min/max statistics, which is exactly what the per-RG RowFilter
+-- skip targets. `skey` is not projected by the query, so the skip also avoids
+-- decoding it on the fully-matched run.
+--
+-- Knobs: PRED_ROWS (row count), RG_SIZE (parquet row-group size).
+set datafusion.execution.parquet.pushdown_filters = true;
+
+COPY (
+ SELECT
+ lpad(CAST(value AS VARCHAR), 10, '0') AS skey,
+ (value * 7) % 1000000 AS p0,
+ (value * 13) % 1000000 AS p1,
+ (value * 17) % 1000000 AS p2,
+ (value * 19) % 1000000 AS p3,
+ (value * 23) % 1000000 AS p4,
+ (value * 29) % 1000000 AS p5,
+ (value * 31) % 1000000 AS p6,
+ (value * 37) % 1000000 AS p7,
+ (value * 41) % 1000000 AS p8,
+ (value * 43) % 1000000 AS p9,
+ (value * 47) % 1000000 AS p10,
+ (value * 53) % 1000000 AS p11,
+ (value * 59) % 1000000 AS p12,
+ (value * 61) % 1000000 AS p13
+ FROM generate_series(1, ${PRED_ROWS:-10000000})
+)
+TO 'sql_benchmarks/parquet_row_filter_skip/scratch/clustered.parquet'
+STORED AS PARQUET
+OPTIONS ('max_row_group_size' '${RG_SIZE:-1000000}');
Review Comment:
The COPY writer option key here is inconsistent with other COPY/Parquet
statement overrides (which use the `format.` prefix). Using
`max_row_group_size` may be rejected as an unknown option or silently ignored,
making `RG_SIZE` ineffective and potentially breaking the benchmark.
Use `format.max_row_group_size` for consistency with existing COPY usage
(e.g. `datafusion/sqllogictest/test_files/copy.slt`).
--
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]
