Support IGNORE NULLS for NTH_VALUE window function #9625
Conversation
github-actions
bot
added
physical-expr
| @@ -249,7 +249,6 @@ fn create_built_in_window_expr( | |||
| .clone() | |||
| .try_into() | |||
| .map_err(|e| DataFusionError::Execution(format!("{e:?}")))?; | |||
| let n: u32 = n as u32; | |||
There was a problem hiding this comment.
I am not sure if we want to support negative index in NTH_VALUE or not. If we want to support, then this line needs to be removed. Otherwise, we probably want to add a check to explicitly block negative index.
There was a problem hiding this comment.
Currently negative index is not supported from an sql query. This means that we reject following kind of queries
SELECT nth_VALUE(column1, -2) OVER(ORDER BY column2 DESC rows between unbounded preceding and current row) FROM t;
This behaviour is similar to Postgres behaviour. However, internally we support negative index. Internal support enables us to rewrite some window expressions by taking their reverse if it helps to remove a SortEXec from the plan.
Consider following query
SELECT nth_VALUE(column1, 2) OVER(ORDER BY column2 DESC rows between unbounded preceding and current row) FROM t;
where it is known that table t is ordered by column2 ASC. We can treat query above as below internally
SELECT nth_VALUE(column1, -2) OVER(ORDER BY column2 ASC rows between current row and unbounded following) FROM t;
which is the same in terms of end result.
In the second version, the ordering requirement for the window function is already satisfied.
In short, we have internal mechanism, to handle negative index. However, it is not exposed to outside users since it is not standart. However, I think there is no harm in having this support. If others agree, we can continue with the changes in this PR.
|
Thanks a lot @Dandandan @mustafasrepo for reviewing and merging the PR! |
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?