jayzhan211 commented on code in PR #26664:
URL: https://github.com/apache/datafusion/pull/26664#discussion_r4365958695
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -464,18 +475,55 @@ impl PartitionEvaluator for NthValueEvaluator {
} else {
// FIRST_VALUE, LAST_VALUE, NTH_VALUE window functions take a
single column, values will have size 1.
let arr = &values[0];
- let n_range = range.end - range.start;
- if n_range == 0 {
- // We produce None if the window is empty.
- return ScalarValue::try_from(arr.data_type());
- }
- match self.valid_index(arr, range) {
- Some(index) => ScalarValue::try_from_array(arr, index),
- None => ScalarValue::try_from(arr.data_type()),
+ match (self.valid_index(arr, range), &self.last_valid) {
+ (Some(index), _) => ScalarValue::try_from_array(arr, index),
+ (None, Some(last_valid)) => Ok(last_valid.clone()),
Review Comment:
Returning `last_valid` for an empty frame looks right, since memoize only
runs with an unbounded start. But no test reaches it: if this arm returns NULL
for an empty range, `window.slt` still passes. The existing tests compute
`max(v)` in the same operator, which keeps the early rows buffered, so the
frame never becomes empty. Could you add a test like this one? It puts the
reference in a separate operator and fails if empty frames return NULL.
```sql
statement ok
set datafusion.execution.batch_size = 2;
# The RANGE frame end stalls after each jump of `k`, so a frame can be empty
# once `memoize` has dropped the rows before it
query II
SELECT count(lv), sum(CASE WHEN lv IS DISTINCT FROM mx THEN 1 ELSE 0 END)
FROM (SELECT mx, LAST_VALUE(v) IGNORE NULLS OVER w AS lv
FROM (SELECT k, v, max(v) OVER w AS mx
FROM (SELECT i + 100 * (i / 10) AS k, CASE WHEN i % 7 < 3 THEN i END AS v
FROM (SELECT value AS i FROM generate_series(1, 200)))
WINDOW w AS (ORDER BY k RANGE BETWEEN UNBOUNDED PRECEDING AND 5
PRECEDING))
WINDOW w AS (ORDER BY k RANGE BETWEEN UNBOUNDED PRECEDING AND 5
PRECEDING));
----
195 0
statement ok
reset datafusion.execution.batch_size;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
