Copilot commented on code in PR #18202:
URL: https://github.com/apache/pinot/pull/18202#discussion_r3230542845
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/value/LeadValueWindowFunction.java:
##########
@@ -88,4 +93,28 @@ public List<Object> processRows(List<Object[]> rows) {
}
return Arrays.asList(result);
}
+
+ /**
+ * LEAD with IGNORE NULLS: for each row, find the offset-th non-null value
scanning forward.
+ * Uses a bounded deque of size {@code _offset} for O(N) time and O(offset)
memory.
+ * Scans right-to-left, maintaining a sliding window of upcoming non-null
values. The oldest
+ * element in the deque (peekFirst) is always the offset-th non-null value
ahead of the current
+ * row.
+ */
+ private List<Object> processRowsIgnoreNulls(List<Object[]> rows) {
+ int numRows = rows.size();
+ Object[] result = new Object[numRows];
+ ArrayDeque<Object> window = new ArrayDeque<>(_offset);
+ for (int i = numRows - 1; i >= 0; i--) {
+ result[i] = (window.size() == _offset) ? window.peekFirst() :
_defaultValue;
+ Object val = extractValueFromRow(rows.get(i));
Review Comment:
`processRowsIgnoreNulls()` assumes `_offset > 0`. If the query specifies
`LEAD(..., 0) IGNORE NULLS`, `window.size() == _offset` is always true and
`peekFirst()` returns null, so the result becomes null for every row (instead
of returning the current row’s value). Also, a negative offset would fail when
constructing `new ArrayDeque<>(_offset)` or via index math in the non-IGNORE
path. Please validate the offset is non-negative (and either explicitly support
`offset = 0` by returning the current row’s value, or reject it with a clear
error).
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/value/LagValueWindowFunction.java:
##########
@@ -88,4 +93,28 @@ public List<Object> processRows(List<Object[]> rows) {
}
return Arrays.asList(result);
}
+
+ /**
+ * LAG with IGNORE NULLS: for each row, find the offset-th non-null value
scanning backward.
+ * Uses a bounded deque of size {@code _offset} for O(N) time and O(offset)
memory.
+ * Scans left-to-right, maintaining a sliding window of preceding non-null
values. The oldest
+ * element in the deque (peekFirst) is always the offset-th non-null value
behind the current
+ * row.
+ */
+ private List<Object> processRowsIgnoreNulls(List<Object[]> rows) {
+ int numRows = rows.size();
+ Object[] result = new Object[numRows];
+ ArrayDeque<Object> window = new ArrayDeque<>(_offset);
+ for (int i = 0; i < numRows; i++) {
+ result[i] = (window.size() == _offset) ? window.peekFirst() :
_defaultValue;
+ Object val = extractValueFromRow(rows.get(i));
Review Comment:
`processRowsIgnoreNulls()` assumes `_offset > 0`. For `LAG(..., 0) IGNORE
NULLS`, `window.size() == _offset` is always true and `peekFirst()` returns
null, producing null for all rows instead of the current row’s value. A
negative offset would also fail (e.g., `new ArrayDeque<>(_offset)` / index
math). Please validate offset is non-negative and either handle `offset = 0` as
an identity or reject it explicitly.
--
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]
