jayzhan211 commented on code in PR #25472:
URL: https://github.com/apache/datafusion/pull/25472#discussion_r4056884283
##########
datafusion/functions-window/src/lead_lag.rs:
##########
Review 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):
```sql
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 20
```
Fix:
```diff
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
}
}
```
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -295,7 +295,13 @@ impl StandardWindowFunctionExpr for WindowUDFExpr {
}
fn limit_effect(&self) -> LimitEffect {
- self.fun.inner().limit_effect(self.args.as_slice())
+ if self.ignore_nulls {
Review 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:
```diff
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.
--
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]
