Skip to content

Commit c7ba69a

Browse files
milindsrivastava1997claudezzylol
authored
feat(frontend-sql): model SQL window frame clauses (ROWS/RANGE BETWEEN ...) (#270)
* feat(frontend-sql): model SQL window frame clauses (ROWS/RANGE BETWEEN ...) SQLWindowFunc previously dropped the frame clause entirely — a default frame, ROWS BETWEEN 2 PRECEDING AND CURRENT ROW, and ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING all lowered to identical IR, so every frame-sensitive window function (SUM, AVG, COUNT, MIN, MAX, FIRST_VALUE, LAST_VALUE, NTH_VALUE) with a non-default frame was silently wrong. Adds WindowFrame/WindowFrameUnits/WindowFrameBound to the canonical IR and threads it through lowering, CSE, resolve, and DAG export. GROUPS frames are rejected explicitly (unused in this repo's SQL corpora, unsupported downstream). Fixes #268 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(sql): preserve range intervals and legacy frames * fix(sql): keep numeric RANGE bounds scalar --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: zz_y <zeyingz@umd.edu>
1 parent 2bd1aeb commit c7ba69a

8 files changed

Lines changed: 330 additions & 10 deletions

File tree

‎crates/frontend-sql/src/sql/mod.rs‎

Lines changed: 82 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
use std::rc::Rc;
2727
use std::sync::Arc;
2828

29+
use datafusion::arrow::compute::kernels::cast_utils::parse_interval_month_day_nano;
2930
use datafusion::arrow::datatypes::{DataType as ArrowDataType, Field};
3031
use datafusion::catalog_common::MemorySchemaProvider;
3132
use datafusion::common::config::ConfigOptions;
@@ -40,6 +41,7 @@ use datafusion::logical_expr::function::{PartitionEvaluatorArgs, WindowUDFFieldA
4041
use datafusion::logical_expr::{
4142
self, lit, AggregateUDF, Case, Distinct, Expr, JoinType, LogicalPlan, PartitionEvaluator,
4243
ScalarUDF, ScalarUDFImpl, Signature, SimpleAggregateUDF, TypeSignature, Volatility,
44+
WindowFrameBound as DfWindowFrameBound, WindowFrameUnits as DfWindowFrameUnits,
4345
WindowFunctionDefinition, WindowUDF, WindowUDFImpl,
4446
};
4547
use datafusion::optimizer::analyzer::function_rewrite::ApplyFunctionRewrites;
@@ -50,7 +52,8 @@ use asap_sql_function_catalog::{AggSemantic, Arity, RewriteKind};
5052
use asap_types::pre_asap::agg_intent::AggIntent;
5153
use asap_types::pre_asap::query_expr::{
5254
GroupKeys, Predicate, ProjectItem, Reduction, SortKey, Source,
53-
UnresolvedQueryExpr as Unresolved,
55+
UnresolvedQueryExpr as Unresolved, WindowFrame, WindowFrameBound, WindowFrameOffset,
56+
WindowFrameUnits,
5457
};
5558
use asap_types::pre_asap::schema::{DataType, Schema};
5659
use asap_types::pre_asap::{
@@ -67,7 +70,7 @@ mod types;
6770
pub use types::SqlCatalog;
6871

6972
use self::expr::df_expr_to_unresolved;
70-
use self::types::{arrow_to_dtype, schema_to_arrow};
73+
use self::types::{arrow_to_dtype, scalar_value_to_asap, schema_to_arrow};
7174

7275
std::thread_local! {
7376
static ACCURACY: std::cell::RefCell<AccuracyTarget> =
@@ -557,8 +560,8 @@ impl<'a> SqlLowerer<'a> {
557560
})
558561
}
559562

560-
/// `func(args) OVER (PARTITION BY … ORDER BY …)`. One window function per
561-
/// plan node; window frames are not modelled yet (default frame assumed).
563+
/// `func(args) OVER (PARTITION BY … ORDER BY … ROWS/RANGE BETWEEN …)`. One
564+
/// window function per plan node.
562565
fn lower_window(&self, window: &logical_expr::Window) -> Result<Unresolved, LoweringError> {
563566
if window.window_expr.len() > 1 {
564567
return Err(LoweringError::UnsupportedFeature(format!(
@@ -613,6 +616,7 @@ impl<'a> SqlLowerer<'a> {
613616
})
614617
})
615618
.collect::<Result<Vec<_>, _>>()?;
619+
let frame = lower_window_frame(&wf.window_frame)?;
616620
// The window plan's schema is `[input fields …, window output]`; the last
617621
// field is the window column's name (what an enclosing Project references).
618622
let output_name = window
@@ -626,6 +630,7 @@ impl<'a> SqlLowerer<'a> {
626630
args,
627631
partition_by: partition_by.into(),
628632
order_by,
633+
frame: Some(frame),
629634
output_name,
630635
child,
631636
})
@@ -1742,6 +1747,79 @@ fn lower_window_func_kind(fun: &WindowFunctionDefinition) -> Result<WindowFuncKi
17421747
}
17431748
}
17441749

1750+
/// Map DataFusion's resolved `WindowFrame` (issue #268) to the canonical
1751+
/// [`WindowFrame`]. DataFusion's planner always fills in the SQL-standard
1752+
/// default frame before the logical plan is built, so this never sees an
1753+
/// "absent" frame — only `ROWS`/`RANGE`/`GROUPS` with concrete bounds.
1754+
/// `GROUPS` is rejected: no query in this repo's SQL corpora uses it, and
1755+
/// nothing downstream interprets frame semantics yet, so it isn't worth
1756+
/// modelling untested.
1757+
fn lower_window_frame(
1758+
frame: &datafusion::logical_expr::WindowFrame,
1759+
) -> Result<WindowFrame, LoweringError> {
1760+
let units = match frame.units {
1761+
DfWindowFrameUnits::Rows => WindowFrameUnits::Rows,
1762+
DfWindowFrameUnits::Range => WindowFrameUnits::Range,
1763+
DfWindowFrameUnits::Groups => {
1764+
return Err(LoweringError::UnsupportedFeature(
1765+
"window frame unit: GROUPS".into(),
1766+
))
1767+
}
1768+
};
1769+
let offset = |v: &DfScalarValue| -> Result<WindowFrameOffset, LoweringError> {
1770+
Ok(match v {
1771+
DfScalarValue::IntervalYearMonth(Some(months)) => WindowFrameOffset::Interval {
1772+
months: *months,
1773+
days: 0,
1774+
nanoseconds: 0,
1775+
},
1776+
DfScalarValue::IntervalDayTime(Some(value)) => WindowFrameOffset::Interval {
1777+
months: 0,
1778+
days: value.days,
1779+
nanoseconds: i64::from(value.milliseconds) * 1_000_000,
1780+
},
1781+
DfScalarValue::IntervalMonthDayNano(Some(value)) => WindowFrameOffset::Interval {
1782+
months: value.months,
1783+
days: value.days,
1784+
nanoseconds: value.nanoseconds,
1785+
},
1786+
// DataFusion 43 keeps every RANGE offset as text: both numeric
1787+
// bounds such as `1.5` and normalized interval literals such as
1788+
// `"1 HOUR"`. Arrow's interval parser accepts bare numbers and
1789+
// interprets them as months, so classify numeric text first.
1790+
DfScalarValue::Utf8(Some(value)) | DfScalarValue::LargeUtf8(Some(value)) => {
1791+
if let Ok(value) = value.parse::<i64>() {
1792+
WindowFrameOffset::Scalar(ScalarValue::Int64(value))
1793+
} else if let Ok(value) = value.parse::<f64>() {
1794+
WindowFrameOffset::Scalar(ScalarValue::Float64(value))
1795+
} else {
1796+
match parse_interval_month_day_nano(value) {
1797+
Ok(interval) => WindowFrameOffset::Interval {
1798+
months: interval.months,
1799+
days: interval.days,
1800+
nanoseconds: interval.nanoseconds,
1801+
},
1802+
Err(_) => WindowFrameOffset::Scalar(scalar_value_to_asap(v)?),
1803+
}
1804+
}
1805+
}
1806+
_ => WindowFrameOffset::Scalar(scalar_value_to_asap(v)?),
1807+
})
1808+
};
1809+
let bound = |b: &DfWindowFrameBound| -> Result<WindowFrameBound, LoweringError> {
1810+
Ok(match b {
1811+
DfWindowFrameBound::Preceding(v) => WindowFrameBound::Preceding(offset(v)?),
1812+
DfWindowFrameBound::CurrentRow => WindowFrameBound::CurrentRow,
1813+
DfWindowFrameBound::Following(v) => WindowFrameBound::Following(offset(v)?),
1814+
})
1815+
};
1816+
Ok(WindowFrame {
1817+
units,
1818+
start_bound: bound(&frame.start_bound)?,
1819+
end_bound: bound(&frame.end_bound)?,
1820+
})
1821+
}
1822+
17451823
// ── Issue #225, item 3: DataFusion registry drift detection ────────────────
17461824
//
17471825
// `asap_sql_function_catalog::NATIVE_FUNCTIONS` is hand-maintained data

‎crates/frontend-sql/tests/sql_lowering.rs‎

Lines changed: 131 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use asap_frontend_sql::{lower_sql, lower_sql_dialect, SqlCatalog};
99
use asap_types::pre_asap::schema::{Column, DataType, Schema};
1010
use asap_types::pre_asap::{
1111
AggIntent, CompareOpKind, GroupKeys, JoinKind, QueryExpr, Reduction, ScalarValue, Source,
12-
WindowFuncKind,
12+
WindowFrameBound, WindowFrameOffset, WindowFrameUnits, WindowFuncKind,
1313
};
1414
use asap_types::types::AccuracyTarget;
1515
use asap_types::workload::SqlDialect;
@@ -722,6 +722,136 @@ async fn window_aggregate_lowers_to_windowfunc() {
722722
assert_eq!(args, &vec![QueryExpr::Column(3)], "SUM(bytes) → arg col 3");
723723
}
724724

725+
// ── Window frames (issue #268) ───────────────────────────────────────────────
726+
727+
/// The frame clause must actually reach the IR, not just the display string:
728+
/// three window frames that differ semantically must lower to different
729+
/// `SQLWindowFunc.frame` values.
730+
#[tokio::test]
731+
async fn window_frame_is_captured_not_dropped() {
732+
let default_frame =
733+
lower("SELECT service, SUM(latency) OVER (PARTITION BY service ORDER BY ts) FROM metrics")
734+
.await;
735+
let two_preceding = lower(
736+
"SELECT service, SUM(latency) OVER (PARTITION BY service ORDER BY ts \
737+
ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM metrics",
738+
)
739+
.await;
740+
let unbounded_following = lower(
741+
"SELECT service, SUM(latency) OVER (PARTITION BY service ORDER BY ts \
742+
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING) FROM metrics",
743+
)
744+
.await;
745+
746+
let frame_of = |qe: &QueryExpr| {
747+
let QueryExpr::SQLWindowFunc { frame, .. } = find_windowfunc(qe).unwrap() else {
748+
unreachable!();
749+
};
750+
frame
751+
.clone()
752+
.expect("newly lowered SQL always records a frame")
753+
};
754+
let (a, b, c) = (
755+
frame_of(&default_frame),
756+
frame_of(&two_preceding),
757+
frame_of(&unbounded_following),
758+
);
759+
assert_ne!(a, b, "default frame vs ROWS 2 PRECEDING must differ");
760+
assert_ne!(
761+
a, c,
762+
"default frame vs ROWS CURRENT..UNBOUNDED FOLLOWING must differ"
763+
);
764+
assert_ne!(b, c);
765+
766+
assert_eq!(b.units, WindowFrameUnits::Rows);
767+
assert_eq!(
768+
b.start_bound,
769+
WindowFrameBound::Preceding(WindowFrameOffset::Scalar(ScalarValue::Int64(2)))
770+
);
771+
assert_eq!(b.end_bound, WindowFrameBound::CurrentRow);
772+
773+
assert_eq!(c.start_bound, WindowFrameBound::CurrentRow);
774+
assert_eq!(
775+
c.end_bound,
776+
WindowFrameBound::Following(WindowFrameOffset::Scalar(ScalarValue::Null))
777+
);
778+
}
779+
780+
#[tokio::test]
781+
async fn range_interval_frame_is_preserved() {
782+
let qe = lower(
783+
"SELECT service, SUM(latency) OVER (PARTITION BY service ORDER BY ts \
784+
RANGE BETWEEN INTERVAL '1' HOUR PRECEDING AND CURRENT ROW) FROM metrics",
785+
)
786+
.await;
787+
let QueryExpr::SQLWindowFunc {
788+
frame: Some(frame), ..
789+
} = find_windowfunc(&qe).unwrap()
790+
else {
791+
panic!("expected a window function with a concrete frame");
792+
};
793+
794+
assert_eq!(frame.units, WindowFrameUnits::Range);
795+
assert_eq!(
796+
frame.start_bound,
797+
WindowFrameBound::Preceding(WindowFrameOffset::Interval {
798+
months: 0,
799+
days: 0,
800+
nanoseconds: 3_600_000_000_000,
801+
})
802+
);
803+
assert_eq!(frame.end_bound, WindowFrameBound::CurrentRow);
804+
}
805+
806+
#[tokio::test]
807+
async fn range_numeric_frames_remain_scalar_offsets() {
808+
let integer = lower(
809+
"SELECT SUM(bytes) OVER (ORDER BY bytes \
810+
RANGE BETWEEN 2 PRECEDING AND CURRENT ROW) FROM metrics",
811+
)
812+
.await;
813+
let fractional = lower(
814+
"SELECT SUM(latency) OVER (ORDER BY latency \
815+
RANGE BETWEEN 1.5 PRECEDING AND CURRENT ROW) FROM metrics",
816+
)
817+
.await;
818+
819+
let start_bound = |qe: &QueryExpr| {
820+
let QueryExpr::SQLWindowFunc {
821+
frame: Some(frame), ..
822+
} = find_windowfunc(qe).unwrap()
823+
else {
824+
panic!("expected a window function with a concrete frame");
825+
};
826+
frame.start_bound.clone()
827+
};
828+
829+
assert_eq!(
830+
start_bound(&integer),
831+
WindowFrameBound::Preceding(WindowFrameOffset::Scalar(ScalarValue::Int64(2)))
832+
);
833+
assert_eq!(
834+
start_bound(&fractional),
835+
WindowFrameBound::Preceding(WindowFrameOffset::Scalar(ScalarValue::Float64(1.5)))
836+
);
837+
}
838+
839+
/// `GROUPS` frames aren't in this repo's SQL corpora and nothing downstream
840+
/// interprets frame semantics yet — rejected explicitly rather than silently
841+
/// mis-lowered.
842+
#[tokio::test]
843+
async fn groups_frame_is_rejected() {
844+
let err = lower_sql(
845+
"SELECT service, SUM(latency) OVER (PARTITION BY service ORDER BY ts \
846+
GROUPS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM metrics",
847+
&catalog(),
848+
AccuracyTarget::Exact,
849+
)
850+
.await
851+
.expect_err("GROUPS frame unit must be rejected");
852+
assert!(format!("{err}").contains("GROUPS"), "got {err}");
853+
}
854+
725855
// ── Nested query functions: derived tables / inline views (issue #27) ───────────
726856

727857
/// Collect every `AggIntent` in the tree, root-to-leaf.

‎crates/types/src/dag_export.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec<DagNode>, cache: &mut HashCache) -> u
499499
args,
500500
partition_by,
501501
order_by,
502+
frame,
502503
output_name,
503504
child,
504505
} => {
@@ -508,6 +509,7 @@ fn build(expr: &QueryExpr, nodes: &mut Vec<DagNode>, cache: &mut HashCache) -> u
508509
"args": args,
509510
"partition_by": partition_by,
510511
"order_by": order_by,
512+
"frame": frame,
511513
"output_name": output_name,
512514
});
513515
push_node(

‎crates/types/src/pre_asap/canonicalize.rs‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,10 @@ fn try_rewrite_rownumber_topk(expr: &QueryExpr) -> Option<QueryExpr> {
294294
#[cfg(test)]
295295
mod tests {
296296
use super::*;
297-
use crate::pre_asap::query_expr::{GroupKeys, ProjectItem, Source};
297+
use crate::pre_asap::query_expr::{
298+
GroupKeys, ProjectItem, Source, WindowFrame, WindowFrameBound, WindowFrameOffset,
299+
WindowFrameUnits,
300+
};
298301
use crate::pre_asap::schema::{Column, DataType, Schema};
299302
use crate::types::AccuracyTarget;
300303

@@ -473,6 +476,16 @@ mod tests {
473476
}
474477
}
475478

479+
/// `ROW_NUMBER` ignores its frame clause, so the top-k rewrite doesn't care
480+
/// what's in it; any concrete frame works as fixture data.
481+
fn rownumber_frame() -> WindowFrame {
482+
WindowFrame {
483+
units: WindowFrameUnits::Rows,
484+
start_bound: WindowFrameBound::Preceding(WindowFrameOffset::Scalar(ScalarValue::Null)),
485+
end_bound: WindowFrameBound::Following(WindowFrameOffset::Scalar(ScalarValue::Null)),
486+
}
487+
}
488+
476489
/// `Filter{ rn(3) <= 5 } { SQLWindowFunc{ RowNumber, PARTITION BY region(2),
477490
/// ORDER BY col(2) DESC } { agg } }`.
478491
fn rownumber_topk(agg: QueryExpr) -> QueryExpr {
@@ -485,6 +498,7 @@ mod tests {
485498
ascending: false,
486499
nulls_first: true,
487500
}],
501+
frame: Some(rownumber_frame()),
488502
output_name: "rn".into(),
489503
child: Rc::new(agg),
490504
};
@@ -567,6 +581,7 @@ mod tests {
567581
ascending: false,
568582
nulls_first: true,
569583
}],
584+
frame: Some(rownumber_frame()),
570585
output_name: "rn".into(),
571586
child: Rc::new(grouped(AggIntent::Count {
572587
accuracy: AccuracyTarget::Exact,

‎crates/types/src/pre_asap/cse.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ pub fn structural_hash(node: &QueryExpr, cache: &mut HashCache) -> u64 {
365365
args,
366366
partition_by,
367367
order_by,
368+
frame,
368369
output_name,
369370
child,
370371
} => {
@@ -376,6 +377,7 @@ pub fn structural_hash(node: &QueryExpr, cache: &mut HashCache) -> u64 {
376377
args,
377378
partition_by,
378379
order_by,
380+
frame,
379381
output_name,
380382
),
381383
);
@@ -682,13 +684,15 @@ fn rebuild_children(table: &mut InternTable, expr: QueryExpr) -> QueryExpr {
682684
args,
683685
partition_by,
684686
order_by,
687+
frame,
685688
output_name,
686689
child,
687690
} => SQLWindowFunc {
688691
func,
689692
args,
690693
partition_by,
691694
order_by,
695+
frame,
692696
output_name,
693697
child: intern_child(table, child),
694698
},

‎crates/types/src/pre_asap/mod.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ pub use query_expr::{
5656
aggregate_output_schema, AtModifier, BinaryOpKind, ColState, DataModel, GroupKeys, GroupSide,
5757
InfoMatcher, JoinKind, Predicate, ProjectItem, QueryExpr, QueryExprError, Reduction,
5858
ResolvedQueryExpr, SampleKind, SetOpKind, SortKey, Source, TimeShift, UnresolvedQueryExpr,
59-
VectorGrouping, VectorMatch, VectorMatchKind, WindowFuncKind,
59+
VectorGrouping, VectorMatch, VectorMatchKind, WindowFrame, WindowFrameBound, WindowFrameOffset,
60+
WindowFrameUnits, WindowFuncKind,
6061
};
6162
pub use resolve::{resolve_root, ResolveTreeError};
6263
pub use schema::{Column, ColumnId, DataType, Schema};

0 commit comments

Comments
 (0)