Repository navigation
Conversation
For every row, `NthValueEvaluator::valid_index` sliced the argument to the frame and enumerated the frame's valid rows from its start to find the last one. A frame that starts at the partition's first row grows with every row, so a partition took time quadratic in its size. Read the frame's validity bits in place, and find the last valid row from the end of the frame, a 64-bit word at a time.
github-actions
Bot
added
sqllogictest
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks @comphead, this is a nice fix with a very clear description. Two things in particular made it easy to review:
- The SLT cases use
max/minover the same frame as an independent oracle, so all 30,000 rows are checked across four NULL patterns and five frame shapes without hand-written expected output. last_set_bit_matches_a_scan_from_the_endsweeps bit offsets 0–69 and lengths 0–200, which reaches every branch ofUnalignedBitChunk::new(≤ 8 bytes, ≤ 16 bytes, andalign_towith and without a prefix or suffix).
What I checked:
-
last_set_bit: I tracedUnalignedBitChunk::newin arrow-buffer 60. In every branch,lead_padding + len + trailing_paddingis 64 × the number of words, and the prefix and suffix masks clear the bits outside the slice. Soword_start + bit - lead_paddingalways lands insidebits. -
Results are unchanged: a throwaway differential test compared the new
valid_indexwith the one from 3ee3e39 on 29.7M cases:- random arrays of up to 320 rows, sliced at bit offsets 0–70
- NULL density from 0% to 100%, including arrays whose validity buffer has no NULLs
- every frame for arrays of up to 90 rows, and random frames above that
FIRST_VALUE,LAST_VALUEandNTH_VALUEwithnin{±1, ±2, ±3, ±7, ±10, ±64, ±65, 130}- both
IGNORE NULLSandRESPECT NULLS
They agreed on every case.
-
Mutation: dropping
- chunk.lead_padding()makes the unit test fail, and makes the new SLT block fail when run on its own (a panic inBooleanBuffer::value), as the description says. The new block passes onmaintoo, which is expected for a perf-only change. -
Tests:
window.sltpasses locally. -
Benchmarks:
cargo bench -p datafusion-functions-window --bench nth_valueon an Apple M3 Max, median of 4 alternating runs of savedmainand PR binaries:Benchmark mainThis PR last_value_expanding/30%_nulls15.78 ms 144 µs last_value_expanding/50%_nulls9.37 ms 144 µs last_value_expanding/80%_nulls3.17 ms 145 µs last_value_sliding_100/50%_nulls723 µs 143 µs first_value_expanding/50%_nulls457 µs 151 µs nth_value_10_expanding/50%_nulls481 µs 183 µs nth_value_neg10_expanding/50%_nulls7.66 ms 8.58 ms first_value_expanding/0%_nulls233 µs 85 µs first_value/respect_nulls59.0 µs 59.0 µs The 0% rows get faster too, because
valid_indexno longer slices the array before checking for a validity buffer.nth_value_neg10_expanding/50%_nullswas slower in each of the 4 rounds here, unlike your M5 Max run. The suggestion inline brings it to about 0.2 ms either way.
Before approving, I'd like the NTH_VALUE(expr, -n) IGNORE NULLS regression on frames without NULLs fixed. It's narrow, but this PR introduces it. Details, numbers and a suggested generalization of last_set_bit are in the inline comment. The generalization also takes care of the second follow-up in your description. Everything else looks good to me.
Follow-up: could you file an issue for the memoize early return? It skips memoizing both LAST_VALUE and NTH_VALUE under IGNORE NULLS, so with an UNBOUNDED PRECEDING frame the frame start never moves and BoundedWindowAggExec buffers the whole partition. That way it won't get lost once this merges.
| Ordering::Less => { | ||
| let reverse_index = (-self.n) as usize; | ||
| let valid_indices_len = nulls.len() - nulls.null_count(); | ||
| let valid_indices_len = valid.count_set_bits(); | ||
| if reverse_index > valid_indices_len { | ||
| return None; | ||
| } | ||
| nulls | ||
| .valid_indices() | ||
| .nth(valid_indices_len - reverse_index) | ||
| .map(|idx| idx + offset) | ||
| valid.set_indices().nth(valid_indices_len - reverse_index) |
There was a problem hiding this comment.
This arm regresses NTH_VALUE(expr, -n) IGNORE NULLS on frames that have no NULLs while the argument still has a validity buffer. On main such a frame failed the null_count() > 0 check and fell through to the O(1) Some(range.end - reverse_index) path. Here every frame takes the bitmap path, so set_indices().nth(valid_indices_len - reverse_index) steps through every valid row of the frame. That's O(frame) bit iteration per row, where main did an O(frame / 64) popcount.
That's the common case with sparse NULLs: most frames have none, but nearly every batch carries a validity buffer. Negative n also comes from the planner, not just from users who write it. get_best_fitting_window reverses a window to reuse an existing sort order, and partition_evaluator then flips the sign of n (is_reversed()). So a sliding NTH_VALUE(x, 3) can run as -3.
From the nth_value bench on an Apple M3 Max. The first three rows are cases I added locally: "0.1% nulls" is create_primitive_array(8192, 0.001), and "null at end" is 8192 rows where only the last one is NULL. Medians of 4 alternating runs (3 for the last column):
| Benchmark | main |
This PR | With nth_set_bit_from_end below |
|---|---|---|---|
nth_value_neg10_sliding_100/0.1%_nulls |
336 µs | 559 µs | 179 µs |
nth_value_neg10_sliding_1000/0.1%_nulls |
2.13 ms | 3.53 ms | 194 µs |
nth_value_neg10_expanding/null_at_end |
395 µs | 13.6 ms | 176 µs |
nth_value_neg10_expanding/30%_nulls |
14.3 ms | 14.2 ms | 186 µs |
nth_value_neg10_expanding/50%_nulls |
7.66 ms | 8.58 ms | 196 µs |
last_value_expanding/50%_nulls |
9.37 ms | 144 µs | 156 µs |
last_value_sliding_100/50%_nulls |
723 µs | 143 µs | 154 µs |
Generalizing last_set_bit to the n-th set bit from the end fixes this, and it also covers the second follow-up in the description:
/// Returns the index of the `n`th set bit from the end of `bits`, where `n = 1` is the last
/// set bit, or `None` if fewer than `n` bits are set.
///
/// The bits are read a 64-bit word at a time from the end, so the cost grows with the distance
/// from the end to that bit rather than with the length of `bits`. Enumerating the set bits
/// from the start instead is quadratic for a frame that grows with every row.
fn nth_set_bit_from_end(bits: &BooleanBuffer, n: usize) -> Option<usize> {
debug_assert!(n > 0, "n is 1-based");
let chunk = UnalignedBitChunk::new(bits.values(), bits.offset(), bits.len());
// The words hold `lead_padding` unset bits before `bits` and `trailing_padding` after it.
let mut word_start = chunk.lead_padding() + bits.len() + chunk.trailing_padding();
let mut remaining = n;
for mut word in chunk.iter().rev() {
word_start -= 64;
let ones = word.count_ones() as usize;
if ones < remaining {
remaining -= ones;
continue;
}
// Clear the `remaining - 1` highest set bits, leaving the one we want on top
for _ in 1..remaining {
word ^= 1 << word.ilog2();
}
return Some(word_start + word.ilog2() as usize - chunk.lead_padding());
}
None
}Call it as NthValueKind::Last => nth_set_bit_from_end(valid, 1) and Ordering::Less => nth_set_bit_from_end(valid, (-self.n) as usize). I checked it three ways:
- your unit test extended to
nin[1, 2, 3, 63, 64, 65, 130]against(0..len).rev().filter(|&i| slice.value(i)).nth(n - 1) - the differential test described in the review summary
window.slt
For LAST_VALUE the extra popcount costs about 1.4 ns per row (the last two rows above). If you'd rather not pay that, Last can keep calling last_set_bit. I used ilog2 because clippy's manual_ilog2 fires on 63 - word.leading_zeros() inside the shift. In last_set_bit the as usize cast only hides the lint.
If you'd rather keep this PR to LAST_VALUE, the smallest fix is to restore main's shortcut in this arm. This also passes the differential test:
if valid_indices_len == valid.len() {
return Some(valid.len() - reverse_index);
}Bench cases I added, with the updated criterion_group!
fn bench_sparse_nulls(c: &mut Criterion) {
let mut group = c.benchmark_group("nth_value_sparse_nulls");
// ~8 NULLs in 8192 rows: a validity buffer exists, but most sliding frames have no NULLs
let sparse =
Arc::new(create_primitive_array::<Int64Type>(ARRAY_SIZE, 0.001)) as ArrayRef;
// Only the last row is NULL: every expanding frame but the last has no NULLs
let null_at_end = Arc::new(
(0..ARRAY_SIZE as i64)
.map(|i| (i != ARRAY_SIZE as i64 - 1).then_some(i))
.collect::<arrow::array::Int64Array>(),
) as ArrayRef;
for window_size in [100usize, 1000] {
group.bench_function(
BenchmarkId::new(
format!("nth_value_neg10_sliding_{window_size}"),
"0.1%_nulls",
),
|b| {
b.iter(|| {
let mut evaluator =
create_evaluator(NthValueKind::Nth, true, Some(-10));
let values_slice = slice::from_ref(&sparse);
for i in 0..sparse.len() {
let start = i.saturating_sub(window_size - 1);
let range = Range { start, end: i + 1 };
black_box(evaluator.evaluate(values_slice, &range).unwrap());
}
})
},
);
}
group.bench_function(
BenchmarkId::new("nth_value_neg10_expanding", "null_at_end"),
|b| {
b.iter(|| {
let mut evaluator = create_evaluator(NthValueKind::Nth, true, Some(-10));
let values_slice = slice::from_ref(&null_at_end);
for i in 0..null_at_end.len() {
let range = Range {
start: 0,
end: i + 1,
};
black_box(evaluator.evaluate(values_slice, &range).unwrap());
}
})
},
);
group.finish();
}
criterion_group!(benches, bench_nth_value_ignore_nulls, bench_sparse_nulls);There was a problem hiding this comment.
Thanks @andygrove, good catch. I took your nth_set_bit_from_end as written in 70fa4d9. It now covers both LAST_VALUE and NTH_VALUE(expr, -n), and the unit test checks n in [1, 2, 3, 63, 64, 65, 130]. I kept a single helper. The extra popcount costs LAST_VALUE about 1 ns per row here too.
nth_value bench on an Apple M5 Max, the previous head against this commit:
| Benchmark | a717eb4 | 70fa4d9 |
|---|---|---|
nth_value_neg10_expanding/30%_nulls |
13.40 ms | 147 µs |
nth_value_neg10_expanding/50%_nulls |
7.64 ms | 156 µs |
nth_value_neg10_expanding/80%_nulls |
2.41 ms | 156 µs |
last_value_expanding/50%_nulls |
113 µs | 120 µs |
last_value_sliding_100/50%_nulls |
116 µs | 126 µs |
window.slt and the extended test suite pass. I filed #26537 for the memoize early return and updated the description.
|
nice improvement for the PR, thanks @andygrove |
…NORE NULLS` Every frame with a validity buffer now takes the bitmap path, so a negative `n` enumerated the frame's valid rows from its start, even when the frame had no NULLs. Generalize `last_set_bit` to `nth_set_bit_from_end` and use it for both `LAST_VALUE` and `NTH_VALUE` with a negative `n`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26533 +/- ##
========================================
Coverage 82.93% 82.94%
========================================
Files 1154 1156 +2
Lines 458002 458532 +530
Branches 458002 458532 +530
========================================
+ Hits 379856 380321 +465
- Misses 55123 55157 +34
- Partials 23023 23054 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
run benchmark nth_value env:
BENCH_FILTER: last_value_expanding|nth_value_neg10_expanding |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing last-value-ignore-nulls-quadratic (70fa4d9) to 3ee3e39 (merge-base) diff Run configurationrun benchmark nth_value
env:
BENCH_FILTER: "last_value_expanding|nth_value_neg10_expanding"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing last-value-ignore-nulls-quadratic (70fa4d9) to 3ee3e39 (merge-base) diff Run configurationrun benchmark nth_value
env:
BENCH_FILTER: "last_value_expanding|nth_value_neg10_expanding"CPU Details (lscpu)Details
Resource Usagenth_value — base (merge-base)
nth_value — branch
File an issue against this benchmark runner |
There was a problem hiding this comment.
Thanks for this optimization. Replacing repeated frame-level null counting with direct bitmap access and reverse scanning is a sound approach, with good regression coverage.
The change addresses the scan cost within NthValueEvaluator, where the relevant frame and validity information is available. It preserves SQL semantics and public APIs while improving the common case for LAST_VALUE and negative NTH_VALUE with IGNORE NULLS.
I found no blocking correctness issues. The rank handling, frame offsets, bitmap boundaries, and empty-frame behavior look consistent. One optional SQL coverage suggestion is included below.
| FROM (SELECT | ||
| NTH_VALUE(v, 1) IGNORE NULLS OVER (PARTITION BY p ORDER BY i) AS n1, | ||
| min(v) OVER (PARTITION BY p ORDER BY i) AS mn, | ||
| NTH_VALUE(v, -1) IGNORE NULLS OVER (PARTITION BY p ORDER BY i) AS nl, |
There was a problem hiding this comment.
Optional: Could we add a sliding-frame SQL test for NTH_VALUE(v, -65) IGNORE NULLS with fewer than, exactly, and more than 65 valid rows and a nonzero frame start? The helper already tests rank 65, but this would verify the full evaluator path against independently calculated expected values.
5 participants
Which issue does this PR close?
LAST_VALUE ... IGNORE NULLSis quadratic in the partition size #26532.Rationale for this change
LAST_VALUE(expr) IGNORE NULLSover a frame that starts at the first row of the partition, such as the defaultRANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW, took time quadratic in the partition size. A 1M-row partition took 89 s, against 0.1 s withRESPECT NULLS. The issue has the reproducer.What changes are included in this PR?
NthValueEvaluator::valid_indexno longer slices the argument array for every row. It reads the frame's validity bits in place, sinceBooleanBuffer::sliceis O(1), and forLAST_VALUEit finds the last valid row with a newlast_set_bit. That function reads the bitmap a 64-bit word at a time from the end of the frame, so the cost per row is the distance back to the last non-NULL row instead of the size of the frame.FIRST_VALUEandNTH_VALUEkeep their forward scans, but they also skip the per-row slice and null count. The results are unchanged: a frame without NULLs still resolves to its first, last or nth row.Two related costs are not changed here:
memoizestill returns early forLAST_VALUEwithIGNORE NULLS, so the frame start never moves andBoundedWindowAggExeckeeps the whole partition buffered. That isLAST_VALUEandNTH_VALUEwithIGNORE NULLSkeep the whole partition buffered #26539.NTH_VALUE(expr, -n) IGNORE NULLSstill counts the frame's valid rows for every row. That isNTH_VALUE(expr, -n) IGNORE NULLSis quadratic in the partition size #26538.What is the testing strategy for this PR?
New
window.sltcases coverLAST_VALUE,FIRST_VALUEandNTH_VALUEwithIGNORE NULLSover partitions that span many 100-row batches. The partitions hold alternating NULLs, NULL runs across several bitmap words, only NULLs, or no NULLs. The cases use growing, sliding, whole-partition and peer-group frames.vincreases within each partition, so every case is checked againstmaxorminover the same frame, which computes the same value through the aggregate path. A unit test checkslast_set_bitagainst a scan from the end for every bit offset up to 70 and every length up to 200. Both kinds of test fail iflast_set_bitignores the lead padding.cargo bench -p datafusion-functions-window --bench nth_value,mainagainst this branch on an Apple M5 Max:mainlast_value_expanding/30%_nullslast_value_expanding/50%_nullslast_value_expanding/80%_nullslast_value_sliding_100/50%_nullsfirst_value_expanding/50%_nullsnth_value_10_expanding/50%_nullsnth_value_neg10_expanding/50%_nullsfirst_value/respect_nullsThe issue's query in
datafusion-cli(release-nonltoprofile), 1M rows:mainAre there any user-facing changes?
No, apart from the speed-up.