Support `IGNORE|RESPECT` NULLs clause in window functions #998

yuval-illumex · 2023-10-11T13:16:02Z

Adding Support for some Rank function of Snowflake:
FIRST_VALUE, LAG, LAST_VALUE,LEAD

iffyio · 2023-10-12T04:45:54Z

tests/sqlparser_snowflake.rs

+    let real_sql = "SELECT col_1, col_2, LAG(col_2) IGNORE NULLS OVER (ORDER BY col_1) FROM t1";
+    assert_eq!(snowflake().verified_stmt(real_sql).to_string(), real_sql);
+
+    let first_sql = "SELECT column1, column2, FIRST_VALUE(column2) OVER (PARTITION BY column1 ORDER BY column2 NULLS LAST) AS column2_first FROM t1";
+    assert_eq!(snowflake().verified_stmt(first_sql).to_string(), first_sql);
+
+    let sql_only_select = "SELECT LAG(col_2,1,0) OVER (PARTITION BY col_3 ORDER BY col_1)";
+    let select = snowflake().verified_only_select(sql_only_select);


It seems this rather extends support for window function options Ignore/Respect NULLs vs snowflake specific, since the options are standard sql (e.g the test first_sql and sql_select_only should already be parsed without the MR)? If so it might be reasonable to instead extend the parse_function() to optionally accept an option where applicable like the following?

enum WindowFunctionOption { RespectNulls, IgnoreNulls }

Since parse_function already contains most of the functionality being added, and prevents window functions from being parsed into potentially different types in the AST which might not be desirable.

Thank you @iffyio !
I extracted the null clause to an enum, and moved it to parse_function.
I will appreciate any feedback.

Thanks!

Oh nice!

One thing that stood out was the mismatch between the field name and the typename:

nulls_clause: Option<WindowFunctionOption>

was curious and took a quick look and it turns out the option is mostly called null treatment clause - maybe we can rename accordingly and so they're consistent?

null_treatment: Option<NullTreatment>

Other than that I think the changes look good!

Thanks @iffyio ! I looked for a better name but didn't found one, you did it!
Renamed :)

coveralls · 2023-10-12T13:07:30Z

Pull Request Test Coverage Report for Build 6626368384

61 of 62 (98.39%) changed or added relevant lines in 12 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 87.415%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	10	11	90.91%

Totals
Change from base Build 6626248059:	0.04%
Covered Lines:	17066
Relevant Lines:	19523

💛 - Coveralls

alamb

Thanks you for the contribution @yuval-illumex -- the basic idea is good but I think it needs a few cleanups

test.sql

src/ast/mod.rs

alamb · 2023-10-20T19:38:28Z

tests/sqlparser_common.rs

@@ -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 [


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

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.

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

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.

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

tests/sqlparser_snowflake.rs

alamb · 2023-10-20T19:40:31Z

Thank you for the review @iffyio -- I appreciate it

yuval-illumex · 2023-10-22T08:37:19Z

Thank you @alamb for your feedback! Please see my comment on the tests

alamb

Thanks @yuval-illumex -- we are getting close I think

alamb · 2023-10-24T11:29:48Z

tests/sqlparser_common.rs

@@ -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 [


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

src/ast/mod.rs

Co-authored-by: Andrew Lamb <[email protected]>

yuval-illumex · 2023-10-24T11:51:26Z

Thanks @alamb - it's ready for another review

alamb

Looks good to me -- thanks again @yuval-illumex and @mustafasrepo

Co-authored-by: Andrew Lamb <[email protected]>

yuval-illumex added 2 commits October 11, 2023 16:13

inital commit

0a7123b

fix test

78cad93

iffyio reviewed Oct 12, 2023

View reviewed changes

yuval-illumex added 5 commits October 12, 2023 13:23

extract enum

c8ac7fd

refactor

3035a79

merge fixes

fa507d8

move null_clause

c8cb2c4

lint

5e5a047

yuval-illumex added 2 commits October 12, 2023 16:09

try to fix tests

25da82a

rename

9819639

alamb changed the title ~~Snowflake - Add Rank Function Support~~ Support IGNORE|RESPECT NULLs clause in window functions Oct 20, 2023

alamb reviewed Oct 20, 2023

View reviewed changes

yuval-illumex added 2 commits October 22, 2023 10:47

delete file

e5b30e5

PR

504a215

alamb reviewed Oct 24, 2023

View reviewed changes

yuval-illumex and others added 3 commits October 24, 2023 14:33

Update src/ast/mod.rs

b7b9b30

Co-authored-by: Andrew Lamb <[email protected]>

merge from main

a914309

maintain old tests

6432c94

alamb approved these changes Oct 24, 2023

View reviewed changes

alamb merged commit b89edaa into apache:main Oct 24, 2023
10 checks passed

serprex pushed a commit to serprex/sqlparser-rs that referenced this pull request Nov 6, 2023

Support IGNORE|RESPECT NULLs clause in window functions (apache#998)

e7af75c

Co-authored-by: Andrew Lamb <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `IGNORE|RESPECT` NULLs clause in window functions #998

Support `IGNORE|RESPECT` NULLs clause in window functions #998

yuval-illumex commented Oct 11, 2023

iffyio Oct 12, 2023

yuval-illumex Oct 12, 2023

iffyio Oct 12, 2023

yuval-illumex Oct 12, 2023

coveralls commented Oct 12, 2023 •

edited

Loading

alamb left a comment

alamb Oct 20, 2023

yuval-illumex Oct 22, 2023

alamb Oct 24, 2023

mustafasrepo Oct 24, 2023 •

edited

Loading

yuval-illumex Oct 24, 2023

alamb commented Oct 20, 2023

yuval-illumex commented Oct 22, 2023

alamb left a comment

alamb Oct 24, 2023

yuval-illumex commented Oct 24, 2023

alamb left a comment

Support IGNORE|RESPECT NULLs clause in window functions #998

Support IGNORE|RESPECT NULLs clause in window functions #998

Conversation

yuval-illumex commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 12, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6626368384

💛 - Coveralls

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustafasrepo Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Oct 20, 2023

yuval-illumex commented Oct 22, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuval-illumex commented Oct 24, 2023

alamb left a comment

Choose a reason for hiding this comment

Support `IGNORE|RESPECT` NULLs clause in window functions #998

Support `IGNORE|RESPECT` NULLs clause in window functions #998

coveralls commented Oct 12, 2023 •

edited

Loading

mustafasrepo Oct 24, 2023 •

edited

Loading