Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25472 +/- ##
==========================================
+ Coverage 81.91% 82.35% +0.43%
==========================================
Files 1135 1137 +2
Lines 427416 432734 +5318
Branches 427416 432734 +5318
==========================================
+ Hits 350128 356359 +6231
+ Misses 56368 54844 -1524
- Partials 20920 21531 +611 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @lyne7-sc , there are 2 suggestions
|
|
||
| fn limit_effect(&self) -> LimitEffect { | ||
| self.fun.inner().limit_effect(self.args.as_slice()) | ||
| if self.ignore_nulls { |
There was a problem hiding this comment.
Blanket Unknown also disables pushdown for IGNORE NULLS functions that never look ahead (positive LAG, first_value/last_value/nth_value within a bounded ROWS frame). LAG(v, 1) IGNORE NULLS OVER (ORDER BY id ROWS ...) LIMIT 1 now plans a full SortExec instead of TopK(fetch=1). Only a row-count effect is invalidated by skipping NULLs:
fn limit_effect(&self) -> LimitEffect {
- if self.ignore_nulls {
+ match self.fun.inner().limit_effect(self.args.as_slice()) {
// The function's offset counts non-null values, so it cannot bound
// the number of input rows needed when NULLs are skipped.
- LimitEffect::Unknown
- } else {
- self.fun.inner().limit_effect(self.args.as_slice())
+ LimitEffect::Relative(_) | LimitEffect::Absolute(_) if self.ignore_nulls => {
+ LimitEffect::Unknown
+ }
+ effect => effect,
}
}Needs the WindowShift::limit_effect change from the other comment so negative-offset LAG reports Relative. With both applied locally, all window*.slt pass and TopK is retained.
| } | ||
|
|
||
| fn limit_effect(&self, args: &[Arc<dyn PhysicalExpr>]) -> LimitEffect { | ||
| if self.kind == WindowShiftKind::Lag { |
There was a problem hiding this comment.
limit_effect returns None for every Lag, but LAG(v, -n) looks ahead like LEAD(v, n). The new "negative-offset LAG" test only covers IGNORE NULLS; without it the result is still wrong (pre-existing, fine as a follow-up, but it's the same function):
SELECT id, LAG(v, -1) OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
FROM (VALUES (1, 10), (2, 20), (3, 30), (4, 40)) AS t(id, v)
ORDER BY id LIMIT 1;
-- returns 1 NULL, expected 1 20Fix:
fn limit_effect(&self, args: &[Arc<dyn PhysicalExpr>]) -> LimitEffect {
- if self.kind == WindowShiftKind::Lag {
- return LimitEffect::None;
- }
- match args {
+ let amount = match args {
[_, expr, ..] => {
let Some(lit) = expr.downcast_ref::<expressions::Literal>() else {
return LimitEffect::Unknown;
};
let ScalarValue::Int64(Some(amount)) = lit.value() else {
return LimitEffect::Unknown; // we should only get int64 from the parser
};
- LimitEffect::Relative((*amount).max(0) as usize)
+ *amount
}
- [_] => LimitEffect::Relative(1), // default value
- _ => LimitEffect::Unknown, // invalid arguments
+ [_] => 1, // default value
+ _ => return LimitEffect::Unknown, // invalid arguments
+ };
+ // LAG(n) looks ahead like LEAD(-n)
+ let lookahead = match self.kind {
+ WindowShiftKind::Lag => amount.saturating_neg(),
+ WindowShiftKind::Lead => amount,
+ };
+ if lookahead > 0 {
+ LimitEffect::Relative(offset_magnitude(lookahead))
+ } else {
+ LimitEffect::None
}
}
Which issue does this PR close?
LEAD/LAG IGNORE NULLSreturns incorrect results acrossNULLgaps and withLIMIT#25471.Rationale for this change
LEAD and negative-offset LAG with IGNORE NULLS can return NULL or a default value even when the requested non-null row exists.
This happens when stateful evaluation fails to refill a partially populated lookahead cache, or when window LIMIT pushdown truncates input before the target row.
What changes are included in this PR?
LimitEffect::Unknownfor IGNORE NULLS window UDF expressions, since non-null offsets cannot bound the number of required input rows.What is the testing strategy for this PR?
Added regression tests in
window.sltfor LEAD/LAG IGNORE NULLS across NULL gaps and batch boundaries, and inwindow_limits.sltfor LIMIT pushdown correctness.Are there any user-facing changes?
Affected IGNORE NULLS window queries now return the correct results. No public API changes.