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

Fix bug in nth_value when ignoreNulls is true and no nulls in values #14042

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Fix bug in nth_value when ignoreNulls is true and no nulls in values #14042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions datafusion/functions-window/src/nth_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,10 @@ impl PartitionEvaluator for NthValueEvaluator {
})
.unwrap_or_default();
if valid_indices.is_empty() {
return ScalarValue::try_from(arr.data_type());
None
Copy link
Contributor

Choose a reason for hiding this comment

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

took me a while to figure out what's happening here; maybe instead check slice.nulls() explicitly and return None if it doesn't exist (i.e. there's no nulls)?

also the pre-existing short-circuiting might be useful in cases where the nulls() index exists but valid_indices() truly is empty (i.e. the array is completely nulls)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed #14063 to track the idea. Thank you 🙏

} else {
Some(valid_indices)
}
Some(valid_indices)
} else {
None
};
Expand Down
38 changes: 35 additions & 3 deletions datafusion/sqllogictest/test_files/window.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1767,7 +1767,7 @@ logical_plan
01)Projection: count(*) AS global_count
02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1)) AS count(*)]]
03)----SubqueryAlias: a
04)------Projection:
04)------Projection:
05)--------Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[]]
06)----------Projection: aggregate_test_100.c1
07)------------Filter: aggregate_test_100.c13 != Utf8("C2GT5KVyOPZpgKVl110TyZO0NcJ434")
Expand Down Expand Up @@ -4625,6 +4625,38 @@ NULL 1
statement ok
DROP TABLE t;


# Test for ignore nulls in nth_VALUE without null values
statement ok
CREATE TABLE t AS VALUES (3, 3), (4, 4), (5, 5), (6, 6);

query I
SELECT column1 FROM t ORDER BY column2;
----
3
4
5
6

query I
SELECT nth_VALUE(column1, 1) OVER(ORDER BY column2) FROM t;
----
3
3
3
3

query I
SELECT nth_VALUE(column1, 1) IGNORE NULLS OVER(ORDER BY column2) FROM t;
----
3
3
3
3

statement ok
DROP TABLE t;

# Test for ignore nulls with ORDER BY in nth_VALUE
statement ok
CREATE TABLE t AS VALUES (3, 3), (4, 4), (null::bigint, 1), (null::bigint, 2), (5, 5), (6, 6);
Expand Down Expand Up @@ -5055,7 +5087,7 @@ select b, row_number() over (order by a) from (select TRUE as a, 1 as b);

# test window functions on boolean columns
statement count 0
create table t1 (id int, bool_col boolean) as values
create table t1 (id int, bool_col boolean) as values
(1, true),
(2, false),
(3, true),
Expand Down Expand Up @@ -5110,7 +5142,7 @@ select ntile(2) over (order by bool_col) from t1;
2

query IIIRRI
select
select
row_number() over (order by bool_col) as row_num,
rank() over (order by bool_col) as rank,
dense_rank() over (order by bool_col) as dense_rank,
Expand Down
Loading