Skip to content

feat: Enable expressions as default value in lead/lag function - #22134

Open
sandugood wants to merge 19 commits into
apache:mainfrom
sandugood:feat/non-literal-default-value-window
Open

feat: Enable expressions as default value in lead/lag function#22134
sandugood wants to merge 19 commits into
apache:mainfrom
sandugood:feat/non-literal-default-value-window

Conversation

@sandugood

@sandugood sandugood commented May 12, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

In the current DataFusion implementation we can only use a ScalarValue as the default value in both LAG and LEAD window functions. That means that users can't reference other columns' values from the table as default value and create various combinations of them.

It means that currently we cannot perform something like:
lead(col1, 1, col1 / col2) as lead_result

What changes are included in this PR?

  1. Added DefaultValue enum that can be either a Literal value or an Expression
  2. Reimplemented parse_default_value function to take default_value's type into consideration
  3. Added two functions: shift_with_array_default and evaluate_all_with_ignore_null_and_array_default for the situations, when default_value is of DefaultValue::Expression type
  4. Added examples for this usecases.

Are these changes tested?

Yes, changes were tested:

  1. Added additional logic tests in window.slt for the sqllogictest

Are there any user-facing changes?

There are user-facing changes in terms of documentation and DataFusion usage (no syntax changes per se). Also, docs were updated

@github-actions github-actions Bot added the functions Changes to functions implementation label May 12, 2026
Comment thread datafusion/functions-window/src/lead_lag.rs Outdated
@sandugood
sandugood marked this pull request as draft May 13, 2026 08:18
@github-actions github-actions Bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels May 13, 2026
@sandugood

Copy link
Copy Markdown
Author

Added the tests in the window.slt file

@sandugood
sandugood marked this pull request as ready for review May 13, 2026 21:11
Comment thread docs/source/user-guide/sql/window_functions.md
@sandugood

Copy link
Copy Markdown
Author

Would be nice if you could review this @comphead, if you have time
Thank you

@github-actions

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale PR has not had any activity for some time label Jul 21, 2026
@Jefffrey Jefffrey removed the Stale PR has not had any activity for some time label Jul 21, 2026
@Jefffrey
Jefffrey requested a review from comphead July 21, 2026 13:20
Comment thread datafusion/sqllogictest/test_files/window.slt Outdated
Comment thread datafusion/functions-window/src/utils.rs Outdated
Comment on lines +792 to +800
let default_array = values.get(1).cloned().unwrap_or_else(|| {
Arc::new(arrow::array::NullArray::new(value.len()))
});
let default_array = if default_array.data_type() != value.data_type() {
arrow::compute::kernels::cast::cast(&default_array, value.data_type())
.map_err(|e| arrow_datafusion_err!(e))?
} else {
default_array
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do wonder if theres a way to pull this casting logic away from execution and into planning, perhaps by changing to a user defined signature for the window function and ensuring the default argument is coerced to the same type as the primary expression

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it to the expressions() method of the WindowShift

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still feel this is something that can be handled via signature planning 🤔

Comment thread datafusion/functions-window/src/lead_lag.rs Outdated
@sandugood

Copy link
Copy Markdown
Author

Thanks for comments, @Jefffrey
Going to address them

@comphead

Copy link
Copy Markdown
Contributor

Thanks @sandugood I will check the PR soon

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 48.00000% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (e8a65f2) to head (97cb216).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-window/src/lead_lag.rs 48.00% 75 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #22134      +/-   ##
==========================================
- Coverage   80.66%   80.65%   -0.02%     
==========================================
  Files        1095     1095              
  Lines      372294   372423     +129     
  Branches   372294   372423     +129     
==========================================
+ Hits       300324   300364      +40     
- Misses      54055    54138      +83     
- Partials    17915    17921       +6     

☔ 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.

@Jefffrey

Copy link
Copy Markdown
Contributor

cc @xudong963 since you worked on optimizing lead/lag recently

# test LAG default is sum of two columns
query II
WITH t(a, b, c) AS (VALUES (1, 10, 100), (2, 20, 200), (3, 30, 300))
SELECT a, lag(a, 1, b + c) OVER (ORDER BY a) FROM t

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@comphead

Copy link
Copy Markdown
Contributor

I made an initial review with assistance of LLM and it comes up with some high severity issues that needed to be checked

LEAD ... IGNORE NULLS with expression default returns default for every row

datafusion/functions-window/src/lead_lag.rs, new evaluate_all_with_ignore_null_and_array_default (diff lines 220-262).

shift_offset for LEAD is negative i64. Both arms do pos.checked_add(offset as usize). Casting a negative i64 to usize wraps to usize::MAX, checked_add overflows to None, and every row falls
back to default_values[id].

The Err(pos) arm is also off-by-one for LEAD independent of the sign bug: it needs pos + k - 1 (the null's insertion point already points at the first at-or-after valid), while Ok(pos) needs pos + k.

SELECT lead(a, 1, b) IGNORE NULLS OVER (ORDER BY id)
FROM (VALUES (1,1,100),(2,NULL,200),(3,3,300)) AS t(id,a,b);
-- returns [100, 200, 300] instead of [3, 3, 300]

.unwrap() in WindowUDFImpl::expressions() can panic

datafusion/functions-window/src/lead_lag.rs, diff lines 112-117.

let main_expr = parse_expr(...).unwrap();
let main_expr_nullable = expr_args.input_fields().first().unwrap().data_type().is_null();

parse_expr returns Result and .first() returns Option. A caller with empty input_fields (optimizer rule rebuilding the plan) panics the executor thread.


expressions() materializes literal defaults on every batch

datafusion/functions-window/src/lead_lag.rs, diff lines 118-141.

expressions() pushes input_exprs[2] (or a CastExpr wrapping it) regardless of whether it is a Literal. Downstream evaluate_expressions_to_arrays calls into_array_of_size(num_rows) for every
expression. In evaluate_all, the DefaultValue::Literal arm then ignores values[1].

lag(x, 1, 0) over a 10M-row partition allocates an extra 10M-element array on every batch and discards it. Regression on the most common form of LAG/LEAD.


Silent fallbacks mask invariant breaks

datafusion/functions-window/src/lead_lag.rs, diff lines 305-316 and 353-356.

// evaluate_all
let default_array = values.get(1).cloned()
    .unwrap_or_else(|| Arc::new(arrow::array::NullArray::new(value.len())));

// evaluate
values.get(1).map(|defaults| { ... })
    .unwrap_or_else(|| ScalarValue::try_from(array.data_type()))

Both paths silently emit NULLs when DefaultValue::Expression disagrees with values.len(). Wrong answer instead of an error.


Test coverage gaps

datafusion/sqllogictest/test_files/window.slt, diff lines 415-742. Missing cases:

  • LAG/LEAD ... IGNORE NULLS with a per-row expression default (would immediately expose finding MINOR: Set GitHub description and labels #1).
  • Per-row default that is NULL for some rows.
  • Main and default with different DataTypes (exercises CastExpr insertion at diff line 134-138).
  • Reversed execution (ORDER BY <col> DESC).
  • Empty partition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support ColumnExpr in LAG/LEAD for default parameter

4 participants