Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support IGNORE|RESPECT NULLs clause in window functions #998

Merged
merged 14 commits into from
Oct 24, 2023
Merged

Support IGNORE|RESPECT NULLs clause in window functions #998

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0a7123b
inital commit
yuval-illumex Oct 11, 2023
78cad93
fix test
yuval-illumex Oct 11, 2023
c8ac7fd
extract enum
yuval-illumex Oct 12, 2023
3035a79
refactor
yuval-illumex Oct 12, 2023
fa507d8
merge fixes
yuval-illumex Oct 12, 2023
c8cb2c4
move null_clause
yuval-illumex Oct 12, 2023
5e5a047
lint
yuval-illumex Oct 12, 2023
25da82a
try to fix tests
yuval-illumex Oct 12, 2023
9819639
rename
yuval-illumex Oct 12, 2023
e5b30e5
delete file
yuval-illumex Oct 22, 2023
504a215
PR
yuval-illumex Oct 22, 2023
b7b9b30
Update src/ast/mod.rs
yuval-illumex Oct 24, 2023
a914309
merge from main
yuval-illumex Oct 24, 2023
6432c94
maintain old tests
yuval-illumex Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,23 @@ impl fmt::Display for WindowFrameUnits {
}
}

#[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
yuval-illumex marked this conversation as resolved.
Show resolved Hide resolved
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub enum NullTreatment {
IgnoreNulls,
RespectNulls,
}

impl fmt::Display for NullTreatment {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str(match self {
NullTreatment::IgnoreNulls => "IGNORE NULLS",
NullTreatment::RespectNulls => "RESPECT NULLS",
})
}
}

/// Specifies [WindowFrame]'s `start_bound` and `end_bound`
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
Expand Down Expand Up @@ -3650,6 +3667,7 @@ impl fmt::Display for CloseCursor {
pub struct Function {
pub name: ObjectName,
pub args: Vec<FunctionArg>,
pub null_treatment: Option<NullTreatment>,
yuval-illumex marked this conversation as resolved.
Show resolved Hide resolved
pub over: Option<WindowType>,
// aggregate functions may specify eg `COUNT(DISTINCT x)`
pub distinct: bool,
Expand Down Expand Up @@ -3698,6 +3716,10 @@ impl fmt::Display for Function {
display_comma_separated(&self.order_by),
)?;

if let Some(o) = &self.null_treatment {
write!(f, " {o}")?;
}

if let Some(o) = &self.over {
write!(f, " OVER {o}")?;
}
Expand Down
1 change: 1 addition & 0 deletions src/ast/visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ where
/// *expr = Expr::Function(Function {
/// name: ObjectName(vec![Ident::new("f")]),
/// args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(old_expr))],
/// null_treatment: None,
/// over: None, distinct: false, special: false, order_by: vec![],
/// });
/// }
Expand Down
1 change: 1 addition & 0 deletions src/keywords.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ define_keywords!(
REPLACE,
REPLICATION,
RESET,
RESPECT,
RESTRICT,
RESULT,
RETAIN,
Expand Down
16 changes: 16 additions & 0 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,7 @@ impl<'a> Parser<'a> {
Ok(Expr::Function(Function {
name: ObjectName(vec![w.to_ident()]),
args: vec![],
null_treatment: None,
over: None,
distinct: false,
special: true,
Expand Down Expand Up @@ -957,6 +958,19 @@ impl<'a> Parser<'a> {
self.expect_token(&Token::LParen)?;
let distinct = self.parse_all_or_distinct()?.is_some();
let (args, order_by) = self.parse_optional_args_with_orderby()?;
let null_treatment = match self.parse_one_of_keywords(&[Keyword::RESPECT, Keyword::IGNORE])
{
Some(keyword) => {
self.expect_keyword(Keyword::NULLS)?;

match keyword {
Keyword::RESPECT => Some(NullTreatment::RespectNulls),
Keyword::IGNORE => Some(NullTreatment::IgnoreNulls),
_ => None,
}
}
None => None,
};
let over = if self.parse_keyword(Keyword::OVER) {
if self.consume_token(&Token::LParen) {
let window_spec = self.parse_window_spec()?;
Expand All @@ -970,6 +984,7 @@ impl<'a> Parser<'a> {
Ok(Expr::Function(Function {
name,
args,
null_treatment,
over,
distinct,
special: false,
Expand All @@ -987,6 +1002,7 @@ impl<'a> Parser<'a> {
Ok(Expr::Function(Function {
name,
args,
null_treatment: None,
over: None,
distinct: false,
special,
Expand Down
8 changes: 8 additions & 0 deletions test.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
SELECT
yuval-illumex marked this conversation as resolved.
Show resolved Hide resolved
c1
FROM
GFC_SOCIAL.SLOT
WHERE
DATE >= DATE(:myvar) -3
AND DATE < DATE(SYSDATE()) -- SELECT
-- :1
1 change: 1 addition & 0 deletions tests/sqlparser_bigquery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@ fn parse_map_access_offset() {
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(
number("0")
))),],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down
4 changes: 4 additions & 0 deletions tests/sqlparser_clickhouse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ fn parse_map_access_expr() {
Value::SingleQuotedString("endpoint".to_string())
))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -89,6 +90,7 @@ fn parse_map_access_expr() {
Value::SingleQuotedString("app".to_string())
))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -138,6 +140,7 @@ fn parse_array_fn() {
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Identifier(Ident::new("x1")))),
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Identifier(Ident::new("x2")))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -196,6 +199,7 @@ fn parse_delimited_identifiers() {
&Expr::Function(Function {
name: ObjectName(vec![Ident::with_quote('"', "myfun")]),
args: vec![],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down
43 changes: 39 additions & 4 deletions tests/sqlparser_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,7 @@ fn parse_select_count_wildcard() {
&Expr::Function(Function {
name: ObjectName(vec![Ident::new("COUNT")]),
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Wildcard)],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand All @@ -895,6 +896,7 @@ fn parse_select_count_distinct() {
op: UnaryOperator::Plus,
expr: Box::new(Expr::Identifier(Ident::new("x"))),
}))],
null_treatment: None,
over: None,
distinct: true,
special: false,
Expand Down Expand Up @@ -1862,6 +1864,7 @@ fn parse_select_having() {
left: Box::new(Expr::Function(Function {
name: ObjectName(vec![Ident::new("COUNT")]),
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Wildcard)],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand All @@ -1887,6 +1890,7 @@ fn parse_select_qualify() {
left: Box::new(Expr::Function(Function {
name: ObjectName(vec![Ident::new("ROW_NUMBER")]),
args: vec![],
null_treatment: None,
over: Some(WindowType::WindowSpec(WindowSpec {
partition_by: vec![Expr::Identifier(Ident::new("p"))],
order_by: vec![OrderByExpr {
Expand Down Expand Up @@ -2259,18 +2263,34 @@ fn parse_agg_with_order_by() {
Box::new(MsSqlDialect {}),
Box::new(AnsiDialect {}),
Box::new(HiveDialect {}),
Box::new(SnowflakeDialect {}),
],
options: None,
};

for sql in [
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these tests changed?

This PR adds support for window functions (aka those that have an OVER clause) which is different than the ORDER BY clause in the aggregate function argument.

I think you should keep the existing tests and add a new one for the IGNORE NULLS / RESPECT NULLs syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @alamb .
Please look at the syntax of FIRST_VALUE (for example) in some of the dialects:
https://docs.snowflake.com/en/sql-reference/functions/first_value
https://learn.microsoft.com/en-us/sql/t-sql/functions/first-value-transact-sql?view=sql-server-ver16
https://www.postgresqltutorial.com/postgresql-window-function/postgresql-first_value-function/

The Over Keyword is a must. so the syntax FIRST_VALUE(x ORDER BY x) is not valid.
Even if the tests pass, those tests are not aligned with the syntax.
That's why I added two arrays of dialects - for each dialect his supported syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Over Keyword is a must. so the syntax FIRST_VALUE(x ORDER BY x) is not valid.

I think that is true when the FIRST_VALUE function is being used as a window function

Some systems allow FIRST_VALUE to be used as normal aggregate functions -- so like

SELECT 
  FIRST_VALUE(amount ORDER BY time), 
  LAST_VALUE(amount ORDER BY time)
FROM 
  t
GROUP BY
  currency

Perhaps @mustafasrepo can offer some perspective as the author of #882

But basically I think we should retain these tests

Copy link
Contributor

@mustafasrepo mustafasrepo Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually FIRST_VALUE and LAST_VALUE can be used as window or aggregate function (similar to SUM, MIN, etc.). In these tests, they are used as aggregate function. Hence these tests should remain as is.
But having the window tests FIRST_VALUE and LAST_VALUE is also helpful. I think you shouldn't modify existing test, and add your test a a new test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alamb and @mustafasrepo :) Learned something about these functions :)
I maintained the old tests

"SELECT FIRST_VALUE(x ORDER BY x) AS a FROM T",
"SELECT FIRST_VALUE(x ORDER BY x) FROM tbl",
"SELECT LAST_VALUE(x ORDER BY x, y) AS a FROM T",
"SELECT LAST_VALUE(x ORDER BY x ASC, y DESC) AS a FROM T",
"SELECT column1, column2, FIRST_VALUE(column2) OVER (PARTITION BY column1 ORDER BY column2 NULLS LAST) AS column2_first FROM t1",
"SELECT column1, column2, FIRST_VALUE(column2) OVER (ORDER BY column2 NULLS LAST) AS column2_first FROM t1",
"SELECT col_1, col_2, LAG(col_2) OVER (ORDER BY col_1) FROM t1",
"SELECT LAG(col_2, 1, 0) OVER (ORDER BY col_1) FROM t1",
"SELECT LAG(col_2, 1, 0) OVER (PARTITION BY col_3 ORDER BY col_1)",
] {
supported_dialects.verified_stmt(sql);
}

let supported_dialects_nulls = TestedDialects {
dialects: vec![Box::new(MsSqlDialect {}), Box::new(SnowflakeDialect {})],
options: None,
};

for sql in [
"SELECT column1, column2, FIRST_VALUE(column2) IGNORE NULLS OVER (PARTITION BY column1 ORDER BY column2 NULLS LAST) AS column2_first FROM t1",
"SELECT column1, column2, FIRST_VALUE(column2) RESPECT NULLS OVER (PARTITION BY column1 ORDER BY column2 NULLS LAST) AS column2_first FROM t1",
"SELECT LAG(col_2, 1, 0) IGNORE NULLS OVER (ORDER BY col_1) FROM t1",
"SELECT LAG(col_2, 1, 0) RESPECT NULLS OVER (ORDER BY col_1) FROM t1",
] {
supported_dialects_nulls.verified_stmt(sql);
}
}

#[test]
Expand Down Expand Up @@ -3332,6 +3352,7 @@ fn parse_scalar_function_in_projection() {
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(
Expr::Identifier(Ident::new("id"))
))],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -3451,6 +3472,7 @@ fn parse_named_argument_function() {
))),
},
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -3482,6 +3504,7 @@ fn parse_window_functions() {
&Expr::Function(Function {
name: ObjectName(vec![Ident::new("row_number")]),
args: vec![],
null_treatment: None,
over: Some(WindowType::WindowSpec(WindowSpec {
partition_by: vec![],
order_by: vec![OrderByExpr {
Expand Down Expand Up @@ -3525,6 +3548,7 @@ fn test_parse_named_window() {
quote_style: None,
}),
))],
null_treatment: None,
over: Some(WindowType::NamedWindow(Ident {
value: "window1".to_string(),
quote_style: None,
Expand All @@ -3550,6 +3574,7 @@ fn test_parse_named_window() {
quote_style: None,
}),
))],
null_treatment: None,
over: Some(WindowType::NamedWindow(Ident {
value: "window2".to_string(),
quote_style: None,
Expand Down Expand Up @@ -4019,6 +4044,7 @@ fn parse_at_timezone() {
quote_style: None,
}]),
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(zero.clone()))],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -4046,6 +4072,7 @@ fn parse_at_timezone() {
quote_style: None,
},],),
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(zero))],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand All @@ -4057,6 +4084,7 @@ fn parse_at_timezone() {
Value::SingleQuotedString("%Y-%m-%dT%H".to_string()),
),),),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -4215,6 +4243,7 @@ fn parse_table_function() {
args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(
Value::SingleQuotedString("1".to_owned()),
)))],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -4366,6 +4395,7 @@ fn parse_unnest_in_from_clause() {
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("2")))),
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("3")))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -4395,6 +4425,7 @@ fn parse_unnest_in_from_clause() {
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("2")))),
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("3")))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand All @@ -4406,6 +4437,7 @@ fn parse_unnest_in_from_clause() {
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("5")))),
FunctionArg::Unnamed(FunctionArgExpr::Expr(Expr::Value(number("6")))),
],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -6878,6 +6910,7 @@ fn parse_time_functions() {
let select_localtime_func_call_ast = Function {
name: ObjectName(vec![Ident::new(func_name)]),
args: vec![],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -7364,6 +7397,7 @@ fn parse_pivot_table() {
args: (vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(
Expr::CompoundIdentifier(vec![Ident::new("a"), Ident::new("amount"),])
))]),
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down Expand Up @@ -7513,6 +7547,7 @@ fn parse_pivot_unpivot_table() {
args: (vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(
Expr::Identifier(Ident::new("population"))
))]),
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down
1 change: 1 addition & 0 deletions tests/sqlparser_hive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ fn parse_delimited_identifiers() {
&Expr::Function(Function {
name: ObjectName(vec![Ident::with_quote('"', "myfun")]),
args: vec![],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down
1 change: 1 addition & 0 deletions tests/sqlparser_mssql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ fn parse_delimited_identifiers() {
&Expr::Function(Function {
name: ObjectName(vec![Ident::with_quote('"', "myfun")]),
args: vec![],
null_treatment: None,
over: None,
distinct: false,
special: false,
Expand Down
Loading
Loading