Support IGNORE NULLS for FIRST/LAST window function #9470

huaxingao · 2024-03-05T18:07:56Z

Which issue does this PR close?

Closes #.
Related #9055

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

huaxingao · 2024-03-05T23:35:22Z

datafusion/physical-expr/src/window/nth_value.rs

    ) -> Result<Self> {
+        if ignore_nulls {
+            return exec_err!("NTH_VALUE ignore_nulls is not supported yet");


I will have a separate PR for this soon.

huaxingao · 2024-03-06T21:13:01Z

cc @viirya @comphead @alamb @mustafasrepo
Could you please review this PR when you have a moment? Thanks a lot!

viirya · 2024-03-06T21:25:29Z

Thanks @huaxingao. I'll review this in few days.

comphead · 2024-03-06T21:46:37Z

datafusion/common/src/scalar/mod.rs

+    ) -> Result<Self> {
+        // If ignoring nulls, find the next non-null index.
+        if ignore_nulls {
+            while index < (array.len() - 1) && !array.is_valid(index) {


it is probably cheaper to get all null indices first? and then find index in the array?

array.nulls().unwrap().valid_indices().collect::<Vec<_>>()

mustafasrepo · 2024-03-07T08:39:34Z

datafusion/sqllogictest/test_files/window.slt

+----
+4
+4
+4
+4


I would expect, result of this query to be

---- NULL NULL 4 4

ran it on duck db with following commands

CREATE TABLE t AS VALUES (3, 4), (4, 3), (null::bigint, 1), (null::bigint, 1); SELECT FIRST_VALUE(col0 ignore nulls) OVER(ORDER BY col1) FROM t;

duckdb produces result above.

mustafasrepo · 2024-03-07T08:44:14Z

datafusion/physical-expr/src/window/nth_value.rs

+
+            let mut valid_indices = Vec::new();
+            if self.ignore_nulls {
+                valid_indices = arr.nulls().unwrap().valid_indices().collect::<Vec<_>>();


I think, the unexpected result I mentioned is related to here. You might need calculate valid_indices within the range not on whole arr. Also, maybe offset handling will be needed on valid_indices onwards once calculation is done within the range.

mustafasrepo · 2024-03-07T08:47:29Z

datafusion/sqllogictest/test_files/window.slt

+3
+
+query I
+SELECT FIRST_VALUE(column1) OVER(ORDER BY column2) FROM t;


Also queries that include ORDER BY <expr> might as well include <expr> in the SELECT. That way query is much more readable, and we can keep track expected behaviour from the result. However, this is purely stylistic.

viirya · 2024-03-07T18:12:44Z

datafusion/physical-expr/src/window/nth_value.rs

@@ -42,6 +42,7 @@ pub struct NthValue {
    /// Output data type
    data_type: DataType,
    kind: NthValueKind,
+    ignore_nulls: bool,


This is also for NTH_VALUE right? Maybe add it to the title?

Oh, nvm, saw your comment below.

viirya · 2024-03-07T18:23:55Z

datafusion/physical-expr/src/window/nth_value.rs

-                NthValueKind::Last => ScalarValue::try_from_array(arr, range.end - 1),
+                NthValueKind::First => {
+                    let index: usize = if self.ignore_nulls {
+                        valid_indices[0]


As @mustafasrepo said, you will pick the first non-null value in the array, not the first non-null value in the range of the array.

viirya · 2024-03-07T18:24:14Z

datafusion/physical-expr/src/window/nth_value.rs

+                }
+                NthValueKind::Last => {
+                    let index = if self.ignore_nulls {
+                        valid_indices[valid_indices.len() - 1]


huaxingao · 2024-03-10T04:02:54Z

datafusion/sqllogictest/test_files/window.slt

+NULL 1
+
+# query II
+# SELECT LAST_VALUE(column1) IGNORE NULLS OVER(ORDER BY column2 DESC NULLS LAST), column2 FROM t;


this test is currently failing and I got

6 6 5 5 3 4 4 3 4 2 NULL 1

I suspect it's because the code is existing here. For OVER(ORDER BY column2 DESC NULLS LAST), i.e. OVER (ORDER BY column2 DESC NULLS LAST RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW), should is_causal be true?

I examined this problem. It seemed like it is related to memoization. Sent a commit to fix this test case (Uncommented the test also). Thanks @huaxingao for triggering this error.

Thanks a lot @mustafasrepo for your help!

alamb

Thank you @huaxingao -- this looks great to me.

Thanks to everyone else who review and contributed comments and fixes 🙏

alamb · 2024-03-11T19:43:53Z

datafusion/sqllogictest/test_files/window.slt

+NULL 4
+
+query II
+SELECT FIRST_VALUE(column1) IGNORE NULLS OVER(ORDER BY column2), column2 FROM t;


the "all nulls" case is very crazy. Nice work testing

huaxingao · 2024-03-12T16:17:02Z

Thank you all very much for helping me with this PR!

github-actions bot added physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Mar 5, 2024

huaxingao marked this pull request as draft March 5, 2024 18:08

huaxingao commented Mar 5, 2024

View reviewed changes

huaxingao marked this pull request as ready for review March 5, 2024 23:43

huaxingao closed this Mar 5, 2024

huaxingao reopened this Mar 5, 2024

comphead reviewed Mar 6, 2024

View reviewed changes

huaxingao closed this Mar 7, 2024

huaxingao reopened this Mar 7, 2024

mustafasrepo reviewed Mar 7, 2024

View reviewed changes

viirya reviewed Mar 7, 2024

View reviewed changes

huaxingao force-pushed the ignore_nulls_window_first branch from 9d9364f to fd82234 Compare March 10, 2024 01:28

huaxingao commented Mar 10, 2024

View reviewed changes

alamb approved these changes Mar 11, 2024

View reviewed changes

Huaxin Gao added 9 commits March 11, 2024 17:30

Support IGNORE NULLS for FIRST/LAST window function

acf5d9d

fix error

656aec4

fix style error

720a0a7

fix clippy error

60ff3fa

add tests for all NULL values

ced9583

address comments

30ef3d2

fix format

cc9f3b4

address comments

7a15263

fix format

49abb8a

mustafasrepo and others added 2 commits March 11, 2024 17:31

Fix commented test case

73a9924

resolve conflicts

7c7aa8b

huaxingao force-pushed the ignore_nulls_window_first branch from ecf8d44 to 7c7aa8b Compare March 12, 2024 00:40

alamb merged commit 92d046d into apache:main Mar 12, 2024
23 checks passed

huaxingao deleted the ignore_nulls_window_first branch March 12, 2024 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support IGNORE NULLS for FIRST/LAST window function #9470

Support IGNORE NULLS for FIRST/LAST window function #9470

huaxingao commented Mar 5, 2024

huaxingao Mar 5, 2024 •

edited

Loading

huaxingao commented Mar 6, 2024

viirya commented Mar 6, 2024

comphead Mar 6, 2024

mustafasrepo Mar 7, 2024

mustafasrepo Mar 7, 2024 •

edited

Loading

mustafasrepo Mar 7, 2024

viirya Mar 7, 2024 •

edited

Loading

viirya Mar 7, 2024

viirya Mar 7, 2024

viirya Mar 7, 2024

huaxingao Mar 10, 2024

mustafasrepo Mar 11, 2024

huaxingao Mar 11, 2024

alamb left a comment

alamb Mar 11, 2024

huaxingao commented Mar 12, 2024

Support IGNORE NULLS for FIRST/LAST window function #9470

Support IGNORE NULLS for FIRST/LAST window function #9470

Conversation

huaxingao commented Mar 5, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

huaxingao Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

huaxingao commented Mar 6, 2024

viirya commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustafasrepo Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Mar 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaxingao commented Mar 12, 2024

huaxingao Mar 5, 2024 •

edited

Loading

mustafasrepo Mar 7, 2024 •

edited

Loading

viirya Mar 7, 2024 •

edited

Loading