[CALCITE-7701] Support IGNORE NULLS for FIRST_VALUE/LAST_VALUE window functions in the enumerable convention - #5178
[CALCITE-7701] Support IGNORE NULLS for FIRST_VALUE/LAST_VALUE window functions in the enumerable convention#5178xuzifu666 wants to merge 3 commits into
Conversation
… functions in the enumerable convention
| from emp": FILTER clause is not supported for window function FIRST_VALUE | ||
| !error | ||
|
|
||
| # [CALCITE-7701] Support IGNORE NULLS for FIRST_VALUE/LAST_VALUE window functions in the enumerable convention |
There was a problem hiding this comment.
These SQL programs had been validated in Oracle: https://onecompiler.com/oracle/44xqcbrnw
| ? Expressions.postIncrementAssign(idx) | ||
| : Expressions.postDecrementAssign(idx); | ||
|
|
||
| final BlockBuilder loopBody = winResult.nestBlock(); |
There was a problem hiding this comment.
Can you add comments showing the generated Java code for each of these statements?
This would make it much easier to maintain the code.
There was a problem hiding this comment.
OK, I had added comments showing the generated code.
The method Javadoc now includes a full pseudo-code snippet of the generated block (for FIRST_VALUE; LAST_VALUE scans backward), and I added inline comments before the loop body and the if (hasRows) { for (...) } wrapper to show what each statement generates.
| /** | ||
| * Implements FIRST_VALUE / LAST_VALUE with IGNORE NULLS by scanning the | ||
| * frame (forward for FIRST_VALUE, backward for LAST_VALUE) and returning | ||
| * the first non-null argument value, or the default value if all rows in |
There was a problem hiding this comment.
What is the "default value"? Isn't it always null?
There was a problem hiding this comment.
You're right, the default value is always null here. I changed the Javadoc from "or the default value if all rows..." to "or null if all rows..." to be explicit.
| Expressions.parameter(0, boxType, | ||
| winResult.currentBlock().newName( | ||
| seekType == SeekType.START ? "first_value" : "last_value")); | ||
| winResult.currentBlock().add(Expressions.declare(0, res, NULL_EXPR)); |
There was a problem hiding this comment.
It seems to be that this is the default value, so it is NULL.
There was a problem hiding this comment.
Yes, the NULL_EXPR passed to Expressions.declare initializes res to null, which is the default value returned when the frame is empty or every row in the frame is null. This matches the behavior in the non-IGNORE NULLS path, where getDefaultValue(info.returnType()) also produces null for these functions. I kept the code as-is because the surrounding Javadoc now explicitly says "or null if all rows in the frame are null", so the intent should be clear from the documentation.
|
| # Verified against Oracle | ||
| # FIRST_VALUE with IGNORE NULLS returns the first non-null value in the frame | ||
| # (or NULL if the frame is empty or all values are null). | ||
| select o, v, |
There was a problem hiding this comment.
all these tests have a finite window with ROWS.
How does this work for unbounded windows or RANGE windows?
You need much better test coverage.
2 participants
jira: https://issues.apache.org/jira/browse/CALCITE-7701