diff --git a/src/cmd/src/bin/query_perf_fixture/case.rs b/src/cmd/src/bin/query_perf_fixture/case.rs index 199ac9f8119..bf9804a121e 100644 --- a/src/cmd/src/bin/query_perf_fixture/case.rs +++ b/src/cmd/src/bin/query_perf_fixture/case.rs @@ -549,17 +549,13 @@ pub(super) enum Distribution { #[serde(rename = "deterministic_wave")] DeterministicWave { min: f64, max: f64 }, /// Wave values with periodic NULLs: every `null_every`-th sample of each - /// series is NULL, and `0` writes no NULLs. - /// - /// The cadence is counted in samples of the series rather than in generated - /// rows, so every series carries the same null ratio on every layout; a row - /// cadence stays constant per series whenever `series_count` is a multiple of - /// `null_every`. + /// series is NULL. The cadence is counted in samples of the series rather + /// than in generated rows. #[serde(rename = "nullable_wave")] NullableWave { min: f64, max: f64, - null_every: usize, + null_every: NonZeroUsize, }, } diff --git a/src/cmd/src/bin/query_perf_fixture/direct_sst.rs b/src/cmd/src/bin/query_perf_fixture/direct_sst.rs index c601331656d..f35a714d260 100644 --- a/src/cmd/src/bin/query_perf_fixture/direct_sst.rs +++ b/src/cmd/src/bin/query_perf_fixture/direct_sst.rs @@ -266,7 +266,7 @@ fn generate_record_batch( }) => { columns.push(Arc::new(Float64Array::from_iter((0..rows).map(|r| { let sample_index = series_sample_index(layout, sst_idx, base_row, r); - if *null_every > 0 && sample_index.is_multiple_of(*null_every) { + if sample_index.is_multiple_of(null_every.get()) { None } else { Some(wave_value(*min, *max, base_row + r)) @@ -774,96 +774,27 @@ mod tests { } } - /// The nullable range-function case must write NULL on every fourth sample of - /// every series and wave values elsewhere, while its dense control table stays - /// NULL-free. + /// The nullable range-function case keeps per-series NULL positions and wave + /// values distinct from its dense control. #[test] - fn nullable_wave_nulls_one_sample_in_every_four_per_series() { - let case: CaseFile = toml::from_str(include_str!( + fn nullable_wave_keeps_null_positions_and_dense_control() { + let mut case: CaseFile = toml::from_str(include_str!( "../../../../../tests/perf/query_cases/promql_nullable_range_fn/case.toml" )) .expect("nullable range-function case should parse"); - let scenario = match &case.scenario { + let scenario = match &mut case.scenario { Scenario::DirectReadableSst(scenario) => scenario, _ => unreachable!("nullable range-function case is a direct SST case"), }; - assert_eq!(2, scenario.tables.len()); + scenario.layout.series_count = NonZeroUsize::new(2).expect("2 is nonzero"); + scenario.layout.rows_per_sst = 8; + scenario.layout.sst_count = 1; + let sst_idx = 3; - let sequence = 1003; let base_row = sst_idx * scenario.layout.rows_per_sst; - let series_count = scenario.layout.series_count.get(); - let samples_per_series = scenario.layout.rows_per_sst / series_count; for table in &scenario.tables { let metadata = Arc::new(build_region_metadata(table, RegionId::from(43))); - let batch = - generate_record_batch(table, &metadata, &scenario.layout, sst_idx, sequence); - let value = table - .columns - .iter() - .find(|column| column.name == "value") - .expect("fixture case has a value field"); - let (min, max, null_every) = match value.distribution.as_ref() { - Some(Distribution::NullableWave { - min, - max, - null_every, - }) => (*min, *max, *null_every), - Some(Distribution::DeterministicWave { min, max }) => (*min, *max, 0), - other => panic!("unexpected value distribution in {}: {other:?}", table.name), - }; - let host = table - .columns - .iter() - .find(|column| column.name == "host") - .expect("fixture case has a host tag"); - let instance = table - .columns - .iter() - .find(|column| column.name == "instance") - .expect("fixture case has an instance tag"); - - let mut expected = (0..scenario.layout.rows_per_sst) - .map(|row| { - let series = series_for_row(&scenario.layout, sst_idx, base_row, row); - let mut tags = HashMap::new(); - tags.insert(host.name.clone(), tag_value(host, series)); - tags.insert(instance.name.clone(), tag_value(instance, series)); - let is_null = null_every > 0 - && series_sample_index(&scenario.layout, sst_idx, base_row, row) - .is_multiple_of(null_every); - ( - encode_dense_primary_key(table, &tags), - timestamp_for_row(&scenario.layout, base_row, row), - sequence, - tag_value(host, series), - tag_value(instance, series), - (!is_null).then(|| wave_value(min, max, base_row + row)), - ) - }) - .collect::>(); - expected.sort_by(|left, right| { - left.0 - .cmp(&right.0) - .then_with(|| left.1.cmp(&right.1)) - .then_with(|| right.2.cmp(&left.2)) - }); - - let timestamps = batch - .column(batch.num_columns() - 4) - .as_any() - .downcast_ref::() - .expect("nanosecond fixture timestamp") - .values() - .to_vec(); - let sequences = batch - .column(batch.num_columns() - 2) - .as_any() - .downcast_ref::() - .expect("fixture sequence column") - .values() - .to_vec(); - let hosts = string_values(&batch, 0); - let instances = string_values(&batch, 1); + let batch = generate_record_batch(table, &metadata, &scenario.layout, sst_idx, 1003); let values = batch .column(2) .as_any() @@ -871,48 +802,38 @@ mod tests { .expect("fixture value column") .iter() .collect::>>(); - let actual = encoded_primary_keys(&batch) - .into_iter() - .zip(timestamps) - .zip(sequences) - .zip(hosts) - .zip(instances) - .zip(values.iter().cloned()) - .map( - |(((((primary_key, timestamp), sequence), host), instance), value)| { - (primary_key, timestamp, sequence, host, instance, value) - }, - ) - .collect::>(); - assert_eq!(expected, actual, "{}", table.name); + let (min, max, null_positions) = match table + .columns + .iter() + .find(|column| column.name == "value") + .expect("fixture case has a value field") + .distribution + .as_ref() + { + Some(Distribution::NullableWave { min, max, .. }) => (*min, *max, &[0][..]), + Some(Distribution::DeterministicWave { min, max }) => (*min, *max, &[][..]), + other => panic!("unexpected value distribution in {}: {other:?}", table.name), + }; - // Sorted by primary key, every series owns `samples_per_series` - // consecutive rows ordered by timestamp, so its NULLs must sit on a - // single residue class of its own sample positions. - assert_eq!(series_count, values.len() / samples_per_series); - for (chunk_index, chunk) in values.chunks(samples_per_series).enumerate() { - if null_every == 0 { - assert!( - chunk.iter().all(Option::is_some), - "{} series chunk {chunk_index} must stay dense", - table.name - ); - continue; - } - let residue = chunk.iter().position(Option::is_none).unwrap_or_else(|| { - panic!("{} series chunk {chunk_index} has no NULL", table.name) - }); + for (series, samples) in values.chunks(4).enumerate() { assert_eq!( - samples_per_series / null_every, - chunk.iter().filter(|value| value.is_none()).count(), - "{} series chunk {chunk_index} NULL count", + null_positions, + samples + .iter() + .enumerate() + .filter_map(|(position, value)| value.is_none().then_some(position)) + .collect::>(), + "{} series {series} NULL positions", table.name ); - for (position, value) in chunk.iter().enumerate() { + for (sample, value) in samples.iter().enumerate() { + if null_positions.contains(&sample) { + continue; + } assert_eq!( - value.is_none(), - position % null_every == residue, - "{} series chunk {chunk_index} sample {position}", + Some(wave_value(min, max, base_row + sample * 2 + series)), + *value, + "{} series {series} sample {sample} wave value", table.name ); } @@ -920,19 +841,21 @@ mod tests { } } - /// The case's window arithmetic counts the NULL slots of every window, so the - /// cadence must also stay on one residue class of a series across SSTs, not - /// only inside a single SST. + /// NULL cadence must continue across unaligned SST boundaries rather than + /// restarting in each SST. #[test] - fn nullable_wave_keeps_one_null_residue_per_series_across_ssts() { - let case: CaseFile = toml::from_str(include_str!( + fn nullable_wave_continues_across_unaligned_ssts() { + let mut case: CaseFile = toml::from_str(include_str!( "../../../../../tests/perf/query_cases/promql_nullable_range_fn/case.toml" )) .expect("nullable range-function case should parse"); - let scenario = match &case.scenario { + let scenario = match &mut case.scenario { Scenario::DirectReadableSst(scenario) => scenario, _ => unreachable!("nullable range-function case is a direct SST case"), }; + scenario.layout.series_count = NonZeroUsize::new(2).expect("2 is nonzero"); + scenario.layout.rows_per_sst = 6; + scenario.layout.sst_count = 3; let table = scenario .tables .iter() @@ -944,14 +867,10 @@ mod tests { .find(|column| column.name == "value") .expect("fixture case has a value field"); let null_every = match value.distribution.as_ref() { - Some(Distribution::NullableWave { null_every, .. }) => *null_every, + Some(Distribution::NullableWave { null_every, .. }) => null_every.get(), other => panic!("nullable_table must use nullable_wave, found {other:?}"), }; - // Without aligned SST boundaries the per-series cadence would shift by one - // slot per SST, and the expected window counts documented in the case would - // stop holding. let samples_per_series = scenario.layout.rows_per_sst / scenario.layout.series_count.get(); - assert_eq!(0, samples_per_series % null_every); let metadata = Arc::new(build_region_metadata(table, RegionId::from(43))); let mut null_positions: HashMap<(String, String), Vec> = HashMap::new(); @@ -991,15 +910,11 @@ mod tests { null_positions.len(), "every series must be represented" ); - let expected_nulls = scenario.layout.sst_count * samples_per_series / null_every; for (series, positions) in &null_positions { - assert_eq!(expected_nulls, positions.len(), "{series:?} null count"); - assert!( - positions - .iter() - .all(|position| position % null_every == positions[0] % null_every), - "{series:?} nulls leave their residue class: {:?}", - &positions[..positions.len().min(6)] + assert_eq!( + &[0, null_every, null_every * 2], + positions.as_slice(), + "{series:?} NULL positions must continue across SSTs" ); } } diff --git a/tests/perf/fixture-format.md b/tests/perf/fixture-format.md index d722065c86d..647d0819a18 100644 --- a/tests/perf/fixture-format.md +++ b/tests/perf/fixture-format.md @@ -73,18 +73,10 @@ or a wave with periodic NULLs: distribution = { kind = "nullable_wave", min = 0.0, max = 100.0, null_every = 4 } ``` -`nullable_wave` also accepts `null_every = 0` to disable NULLs. It writes NULL on -every `null_every`-th sample of each series and wave values everywhere else, so a -NULL is one missing sample of one series rather than a missing row. The cadence -is counted in samples of the series, not in generated rows, so every series -carries the same null ratio under any `series_layout`; when one SST holds a -multiple of `null_every` samples per series (`rows_per_sst / series_count`), the -pattern also repeats with the same period in every SST. Counting the cadence in -generated rows instead would make it constant per series whenever `series_count` -is a multiple of `null_every` (128 and 4, for example), leaving series either -entirely NULL or entirely NULL-free. Counting per series keeps NULL and non-NULL -samples mixed inside range-function windows, which is what the nullable branches -of the PromQL range functions observe. +`null_every` must be greater than zero. `nullable_wave` writes NULL on every +`null_every`-th sample of each series and wave values everywhere else; the cadence +is counted in per-series samples, not generated rows. Do not use a row-based +cadence: it can leave individual series entirely NULL or entirely NULL-free. ### Query kinds diff --git a/tests/perf/query_cases/promql_nullable_range_fn/case.toml b/tests/perf/query_cases/promql_nullable_range_fn/case.toml index 9ef693a129b..aafbea08a9f 100644 --- a/tests/perf/query_cases/promql_nullable_range_fn/case.toml +++ b/tests/perf/query_cases/promql_nullable_range_fn/case.toml @@ -1,24 +1,6 @@ -# PromQL nullable range-function benchmark: regression evidence for the indexed -# valid-sample lookup in nullable range functions (#9234). -# -# `rate`/`increase`/`delta` and the presence evaluator behind -# `count_over_time`/`last_over_time` fall back to a per-window validity scan once -# any sample of a series is NULL. `nullable_table` writes NULL on every fourth -# sample of every series, so 25% of its samples are missing (one missing sample -# per minute at the 15s cadence) and every window of every series contains both -# NULL and non-NULL samples instead of being entirely NULL or entirely dense. -# `dense_table` is the same fixture without NULLs; it keeps the no-NULL fast path -# measured next to the nullable path. -# -# 128 series x 780 timestamps at 15s cadence = 99,840 rows per table, written as -# 13 SSTs x 7,680 rows. Data starts one hour before evaluation; 481 evaluations -# span two hours. Range windows are left-open and every series is dense on the -# 15s grid, so a fully covered 5m window holds 20 slots and a 1m window holds 4. -# Five of the 20 slots and one of the 4 slots are NULL, which makes -# `count_over_time(nullable_table[5m])` return exactly 15 and -# `count_over_time(nullable_table[1m])` return exactly 3 at every one of the 481 -# evaluations: no window is entirely NULL and none is NULL-free, so every window -# of every series takes the nullable branch. +# Benchmarks nullable PromQL range functions against a dense control. Every +# 15-second series has NULL every fourth sample; fully covered windows therefore +# retain 15 valid samples at [5m] and 3 at [1m]. [case] name = "promql_nullable_range_fn" @@ -115,16 +97,6 @@ iterations = 9 [scenario.queries.thresholds] max_candidate_latency_regression_pct = 20 -[[scenario.queries]] -name = "increase_nullable_5m" -kind = "tql" -query = "TQL ANALYZE VERBOSE (1704070800, 1704078000, '15s') increase(nullable_table{host=~'host.*'}[5m])" -warmup = 3 -iterations = 9 - -[scenario.queries.thresholds] -max_candidate_latency_regression_pct = 20 - # `delta` shares the nullable window-bounds path but skips the counter correction. [[scenario.queries]] name = "delta_nullable_5m"