Summary
The bgp_jan2024_workload 200-query corpus (#219) has 5 occurrences of ClickHouse's lagInFrame, which currently fails to lower — lower_window_func_kind (crates/frontend-sql/src/sql/mod.rs, ~line 1391) is a closed hand-match over WindowFunctionDefinition::WindowUDF names (row_number, rank, dense_rank, lag, lead, first_value, last_value, nth_value) that rejects anything else via LoweringError::UnsupportedFeature.
Why this isn't a simple catalog addition (unlike #230)
lagInFrame's actual semantic difference from plain LAG is that it respects the window frame bounds (returns NULL/a default when the lookback would cross the frame boundary, rather than reaching arbitrarily far back like ordinary LAG). QueryExpr::SQLWindowFunc / WindowFuncKind (crates/types/src/pre_asap/query_expr.rs) has no frame representation at all — its own doc comment says "Window frames are not modelled yet."
So this needs a design decision before it's mechanical:
- Model window frames properly first, then give
lagInFrame its own correct semantics (bigger: touches SQLWindowFunc's shape, output_schema, and downstream binding). This is a prerequisite for correctly supporting ClickHouse's whole *InFrame function family, not just this one name, and possibly needed for SQL's own OVER (... ROWS BETWEEN ...) frame clauses generally, which are presumably in the same boat.
- Map
lagInFrame → WindowFuncKind::Lag as an approximation and accept the semantic gap (silently wrong at the frame boundary) until (1) happens. Cheap, but a correctness compromise that should be a deliberate, documented decision, not a quiet shortcut.
Scope of this issue
Settle which of the two directions above (or another) to take, and size the "model window frames properly" work if that's the chosen direction. Not asking for an implementation yet.
Related
Summary
The
bgp_jan2024_workload200-query corpus (#219) has 5 occurrences of ClickHouse'slagInFrame, which currently fails to lower —lower_window_func_kind(crates/frontend-sql/src/sql/mod.rs, ~line 1391) is a closed hand-match overWindowFunctionDefinition::WindowUDFnames (row_number,rank,dense_rank,lag,lead,first_value,last_value,nth_value) that rejects anything else viaLoweringError::UnsupportedFeature.Why this isn't a simple catalog addition (unlike #230)
lagInFrame's actual semantic difference from plainLAGis that it respects the window frame bounds (returnsNULL/a default when the lookback would cross the frame boundary, rather than reaching arbitrarily far back like ordinaryLAG).QueryExpr::SQLWindowFunc/WindowFuncKind(crates/types/src/pre_asap/query_expr.rs) has no frame representation at all — its own doc comment says "Window frames are not modelled yet."So this needs a design decision before it's mechanical:
lagInFrameits own correct semantics (bigger: touchesSQLWindowFunc's shape,output_schema, and downstream binding). This is a prerequisite for correctly supporting ClickHouse's whole*InFramefunction family, not just this one name, and possibly needed for SQL's ownOVER (... ROWS BETWEEN ...)frame clauses generally, which are presumably in the same boat.lagInFrame→WindowFuncKind::Lagas an approximation and accept the semantic gap (silently wrong at the frame boundary) until (1) happens. Cheap, but a correctness compromise that should be a deliberate, documented decision, not a quiet shortcut.Scope of this issue
Settle which of the two directions above (or another) to take, and size the "model window frames properly" work if that's the chosen direction. Not asking for an implementation yet.
Related
bgp_jan2024_workloadcorpus test, where this gap was foundcrates/types/src/pre_asap/query_expr.rs—QueryExpr::SQLWindowFunc,WindowFuncKindcrates/frontend-sql/src/sql/mod.rs—lower_window_func_kind