yashmayya commented on code in PR #14264:
URL: https://github.com/apache/pinot/pull/14264#discussion_r1818438677
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/value/FirstValueWindowFunction.java:
##########
@@ -78,12 +86,73 @@ private List<Object> processRowsWindow(List<Object[]> rows)
{
lowerBound++;
upperBound = Math.min(upperBound + 1, numRows - 1);
}
+
+ return result;
+ }
+
+ private List<Object> processRowsWindowIgnoreNulls(List<Object[]> rows) {
+ int numRows = rows.size();
+ int lowerBound = _windowFrame.getLowerBound();
+ int upperBound = Math.min(_windowFrame.getUpperBound(), numRows - 1);
+
+ int indexOfFirstNonNullValue = -1;
+ // Find first non-null value in the first window
+ for (int i = Math.max(lowerBound, 0); i <= upperBound; i++) {
+ Object value = extractValueFromRow(rows.get(i));
+ if (value != null) {
+ indexOfFirstNonNullValue = i;
+ break;
+ }
+ }
+
+ List<Object> result = new ArrayList<>();
+
+ for (int i = 0; i < numRows; i++) {
+ if (lowerBound >= numRows) {
+ // Fill the remaining rows with null
+ for (int j = i; j < numRows; j++) {
+ result.add(null);
+ }
+ break;
+ }
+
+ if (indexOfFirstNonNullValue != -1) {
+ result.add(extractValueFromRow(rows.get(indexOfFirstNonNullValue)));
+ } else {
+ result.add(null);
+ }
+
+ // Slide the window forward by one row
+ if (indexOfFirstNonNullValue == lowerBound) {
Review Comment:
Good catch, we'd be doing a pointless iteration from 0 to upper bound when
we're at `lowerBound = -1` 👍
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -326,6 +326,31 @@ public void testAggregateServerReturnFinalResult(boolean
useMultiStageQueryEngin
assertTrue(response.get("resultTable").get("rows").get(0).get(0).isNull());
}
+ @Test
+ public void testWindowFunctionIgnoreNulls()
+ throws Exception {
+ // Window functions are only supported in the multi-stage query engine
+ setUseMultiStageQueryEngine(true);
+ String sqlQuery =
+ "SELECT salary, LAST_VALUE(salary) IGNORE NULLS OVER (ORDER BY
DaysSinceEpoch) AS gapfilledSalary from "
Review Comment:
Yeah, good point, I'll make sure to add a note about this to the
documentation. I plan to raise one consolidated documentation PR with changes
from https://github.com/apache/pinot/pull/14273 and here.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/window/value/FirstValueWindowFunction.java:
##########
@@ -78,12 +86,73 @@ private List<Object> processRowsWindow(List<Object[]> rows)
{
lowerBound++;
upperBound = Math.min(upperBound + 1, numRows - 1);
}
+
+ return result;
+ }
+
+ private List<Object> processRowsWindowIgnoreNulls(List<Object[]> rows) {
Review Comment:
I don't think there's much benefit to that here? In the unbounded case for
`FIRST_VALUE` / `LAST_VALUE` with `IGNORE NULLS`, we'll find the first / last
non-null value in the first window (which will encompass all rows) at the
beginning and then in each iteration there's only some simple boolean checks
which will all be `false` in every iteration of the loop and we'll simply keep
adding the same value to the result list.
--
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]
