fix: correct LEAD/LAG IGNORE NULLS evaluation and limit pushdown

[lyne7-sc]

Which issue does this PR close?

• Closes LEAD/LAG IGNORE NULLS returns incorrect results across NULL gaps and with LIMIT #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?

• Resume scanning after the last cached non-null row until enough candidates are available or the range ends.
• Return LimitEffect::Unknown for 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.slt for LEAD/LAG IGNORE NULLS across NULL gaps and batch boundaries, and in window_limits.slt for LIMIT pushdown correctness.

Are there any user-facing changes?

Affected IGNORE NULLS window queries now return the correct results. No public API changes.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.42%. Comparing base (7917a9a) to head (b5b535a).
⚠️ Report is 144 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/functions-window/src/lead_lag.rs	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25472      +/-   ##
==========================================
+ Coverage   81.91%   82.42%   +0.50%     
==========================================
  Files        1135     1138       +3     
  Lines      427416   435491    +8075     
  Branches   427416   435491    +8075     
==========================================
+ Hits       350128   358934    +8806     
+ Misses      56368    54847    -1521     
- Partials    20920    21710     +790     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jayzhan211]

Thanks @lyne7-sc , there are 2 suggestions

[jayzhan211]

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.

[lyne7-sc]

Updated this to preserve None and only convert Relative/Absolute to Unknown for IGNORE NULLS.

[jayzhan211]

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 20

Fix:

     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
         }
     }

[lyne7-sc]

Fixed this here as well. I reused the existing WindowShiftKind::shift_offset helper.

[lyne7-sc]

Thanks for the detailed review @jayzhan211. I've addressed both comments and added regression slt coverage.

[github-actions]

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-functions-window v55.1.0 (current)
       Built [  35.789s] (current)
     Parsing datafusion-functions-window v55.1.0 (current)
      Parsed [   0.014s] (current)
    Building datafusion-functions-window v55.1.0 (baseline)
       Built [  26.521s] (baseline)
     Parsing datafusion-functions-window v55.1.0 (baseline)
      Parsed [   0.015s] (baseline)
    Checking datafusion-functions-window v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.141s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  63.361s] datafusion-functions-window
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  39.650s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.182s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  39.481s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.181s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.049s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron

Failed in:
  LimitedBatchCoalescer::push_batch_with_filter, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/5b7c95f4b3514f11f6eac123768c5c62c0d95162/datafusion/physical-plan/src/coalesce/mod.rs:128

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  81.898s] datafusion-physical-plan
    Building datafusion-sqllogictest v55.1.0 (current)
       Built [ 102.018s] (current)
     Parsing datafusion-sqllogictest v55.1.0 (current)
      Parsed [   0.025s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [ 101.052s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.025s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.128s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 205.954s] datafusion-sqllogictest

[jayzhan211]

Thanks @lyne7-sc LGTM!