kosiew commented on code in PR #23653:
URL: https://github.com/apache/datafusion/pull/23653#discussion_r3654510693
##########
datafusion/core/benches/parquet_struct_query.rs:
##########
@@ -60,7 +59,7 @@ fn schema() -> SchemaRef {
}
fn generate_strings(len: usize) -> ArrayRef {
- let mut rng = rng();
+ let mut rng = StdRng::seed_from_u64(0);
Review Comment:
`generate_file()` calls `generate_batch(batch_id)` in a loop, but
`generate_strings()` now resets its RNG to seed `0` for every batch. The
top-level IDs still vary, while the string payload inside the structs repeats
identically across all batches.
That changes the nested-column data shape used by the benchmark. Could we
thread a seeded RNG through `generate_file()` and `generate_batch()`, or derive
the seed from `batch_id`, so the payload stays deterministic but does not
repeat?
##########
datafusion/core/benches/parquet_query_sql.rs:
##########
@@ -98,7 +97,7 @@ fn generate_string_dictionary(
len: usize,
valid_percent: f64,
) -> ArrayRef {
- let mut rng = rng();
+ let mut rng = StdRng::seed_from_u64(0);
Review Comment:
`generate_batch()` is called 2048 times, but each random column helper now
creates a new `StdRng::seed_from_u64(0)`. As a result, every generated record
batch contains the same dictionary, string, and primitive values.
This changes the benchmark fixture from a randomized 2 million row Parquet
file into repeated 1024-row chunks, which can affect Parquet encoding,
compression, and scan behaviour. Could we keep one seeded RNG for the file or
batch generation path, or derive stable seeds for each batch and column? That
would keep the benchmark deterministic without losing data diversity.
##########
datafusion/functions/benches/concat.rs:
##########
@@ -49,7 +49,7 @@ fn create_array_args_view(size: usize) -> Vec<ColumnarValue> {
}
fn generate_random_string(str_len: usize) -> String {
- rand::rng()
+ StdRng::seed_from_u64(0)
Review Comment:
A small suggestion: could `generate_random_string()` accept `&mut StdRng`
instead of creating a freshly seeded RNG internally? That would keep the scalar
inputs deterministic without producing ten identical strings, and it would
match the threaded RNG approach used by most of the other updated benchmarks.
##########
benchmarks/src/cancellation.rs:
##########
@@ -312,12 +311,12 @@ async fn generate_data(
}
fn random_data(column_type: &DataType, rows: usize) -> Arc<dyn Array> {
- let mut rng = rand::rng();
+ let mut rng = StdRng::seed_from_u64(0);
Review Comment:
`random_data()` is called separately for every column in every generated
file, and each call now starts from seed `0`. This means columns of the same
type receive identical values. For example, `A`, `B`, and `C` share the same
`Float64` sequence, all UTF-8 columns share the same string sequence, and the
same contents are repeated across files.
Since the cancellation benchmark runs `SELECT distinct "A", "B", "C", "D",
"E"`, this materially changes the workload. Could we use one seeded RNG for the
full dataset generation, or derive stable seeds from values such as `(file_num,
column_name)`?
--
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]
