Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,14 @@ private static void sampleOfTheGeneratedWindowedAggregate() {
for (int aggIdx = 0; aggIdx < aggregateCalls.size(); aggIdx++) {
AggregateCall call = aggregateCalls.get(aggIdx);
if (call.ignoreNulls()) {
throw new UnsupportedOperationException("IGNORE NULLS not supported");
switch (call.getAggregation().getKind()) {
case FIRST_VALUE:
case LAST_VALUE:
// IGNORE NULLS is implemented for these functions below.
break;
default:
throw new UnsupportedOperationException("IGNORE NULLS not supported");
}
}
aggs.add(new AggImpState(aggIdx, call, true, implementorTable));
}
Expand Down Expand Up @@ -821,6 +828,10 @@ private void declareAndResetState(final JavaTypeFactory typeFactory,
@Override public RexWindowExclusion getExclude() {
return exclusion;
}

@Override public boolean ignoreNulls() {
return agg.call.ignoreNulls();
}
};
String aggName = "a" + agg.aggIdx;
if (CalciteSystemProperty.DEBUG.value()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.calcite.linq4j.tree.OptimizeShuttle;
import org.apache.calcite.linq4j.tree.ParameterExpression;
import org.apache.calcite.linq4j.tree.Primitive;
import org.apache.calcite.linq4j.tree.Types;
import org.apache.calcite.linq4j.tree.UnsignedType;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
Expand Down Expand Up @@ -2489,12 +2490,107 @@
AggResultContext result) {
WinAggResultContext winResult = (WinAggResultContext) result;

final boolean ignoreNulls =
info instanceof WinAggContext && ((WinAggContext) info).ignoreNulls();
if (ignoreNulls) {
return implementResultIgnoreNulls(info, winResult);
}

return Expressions.condition(winResult.hasRows(),
winResult.rowTranslator(
winResult.computeIndex(Expressions.constant(0), seekType))
.translate(winResult.rexArguments().get(0), info.returnType()),
getDefaultValue(info.returnType()));
}

/**
* 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 null if all rows in the frame are
* null (or the frame is empty).
*
* <p>Generated code (for FIRST_VALUE; LAST_VALUE scans backward):
* <pre>{@code
* BoxType res = null;
* if (hasRows) {
* for (int seekIdx = startIndex; seekIdx <= endIndex; seekIdx++) {
* BoxType seekValue = rowTranslator.translate(arg, boxType);
* if (seekValue != null) {
* res = seekValue;
* break;
* }
* }
* }
* return res;
* }</pre>
*/
private Expression implementResultIgnoreNulls(AggContext info,
WinAggResultContext winResult) {
final Type returnType = info.returnType();
final RexNode arg = winResult.rexArguments().get(0);

// Use a boxed type internally so that a NULL comparison is always valid,
// even when the (frame-guaranteed non-empty) return type is a primitive.
// The surrounding window implementation converts the result back to the
// declared return type.
final Type boxType = Types.box(returnType);

final ParameterExpression res =
Expressions.parameter(0, boxType,
winResult.currentBlock().newName(
seekType == SeekType.START ? "first_value" : "last_value"));
// res = null
winResult.currentBlock().add(Expressions.declare(0, res, NULL_EXPR));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be that this is the default value, so it is NULL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


final ParameterExpression idx =
Expressions.parameter(int.class,
winResult.currentBlock().newName("seekIdx"));

// Scan direction: FIRST_VALUE walks from start to end, LAST_VALUE walks
// from end back to start.
final boolean forward = seekType == SeekType.START;
final Expression from =
forward ? winResult.startIndex() : winResult.endIndex();
final Expression to =
forward ? winResult.endIndex() : winResult.startIndex();
final Expression condition =
forward
? Expressions.lessThanOrEqual(idx, to)
: Expressions.greaterThanOrEqual(idx, to);
final Expression post =
forward
? Expressions.postIncrementAssign(idx)
: Expressions.postDecrementAssign(idx);

// Build the loop body:
// BoxType seekValue = rowTranslator.translate(arg, boxType);

Check warning on line 2566 in core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This block of commented-out lines of code should be removed.

See more on https://sonarcloud.io/project/issues?id=apache_calcite&issues=AaAAwWm9TV2yRQvmK0WB&open=AaAAwWm9TV2yRQvmK0WB&pullRequest=5178
// if (seekValue != null) {
// res = seekValue;
// break;
// }
final BlockBuilder loopBody = winResult.nestBlock();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comments showing the generated Java code for each of these statements?
This would make it much easier to maintain the code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

final Expression value =
winResult.rowTranslator(idx).translate(arg, boxType);
final ParameterExpression valueVar =
Expressions.parameter(0, boxType, loopBody.newName("seekValue"));
loopBody.add(Expressions.declare(0, valueVar, value));
loopBody.add(
Expressions.ifThen(
Expressions.notEqual(valueVar, NULL_EXPR),
Expressions.block(
Expressions.statement(Expressions.assign(res, valueVar)),
Expressions.break_(null))));
winResult.exitBlock();
final BlockStatement loopBodyBlock = loopBody.toBlock();

// Wrap the scan in: if (hasRows) { for (...) { ... } }

Check warning on line 2586 in core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This block of commented-out lines of code should be removed.

See more on https://sonarcloud.io/project/issues?id=apache_calcite&issues=AaAAwWm9TV2yRQvmK0WC&open=AaAAwWm9TV2yRQvmK0WC&pullRequest=5178
winResult.currentBlock().add(
Expressions.ifThen(winResult.hasRows(),
Expressions.for_(
Expressions.declare(0, idx, from),
condition, post, loopBodyBlock)));
return res;
}
}

/** Implementor for the {@code FIRST_VALUE} windowed aggregate function. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,9 @@
public interface WinAggContext extends AggContext {
/** The exclude clause of the group of the window function. */
RexWindowExclusion getExclude();

/** Whether the window function ignores NULL values (IGNORE NULLS). */
default boolean ignoreNulls() {
return false;
}
}
72 changes: 72 additions & 0 deletions core/src/test/resources/sql/winagg.iq
Original file line number Diff line number Diff line change
Expand Up @@ -1323,4 +1323,76 @@ java.sql.SQLException: Error while executing SQL "select first_value(sal) filter
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These SQL programs had been validated in Oracle: https://onecompiler.com/oracle/44xqcbrnw

# 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

first_value(v) ignore nulls over (order by o rows 2 preceding) as fv
from (values (1, 1), (2, cast(null as integer)), (3, 3),
(4, cast(null as integer)), (5, cast(null as integer))) as t(o, v);
+---+---+----+
| O | V | FV |
+---+---+----+
| 1 | 1 | 1 |
| 2 | | 1 |
| 3 | 3 | 1 |
| 4 | | 3 |
| 5 | | 3 |
+---+---+----+
(5 rows)

!ok

# LAST_VALUE with IGNORE NULLS returns the last non-null value in the frame.
select o, v,
last_value(v) ignore nulls over (order by o rows 2 preceding) as lv
from (values (1, 1), (2, cast(null as integer)), (3, 3),
(4, cast(null as integer)), (5, cast(null as integer))) as t(o, v);
+---+---+----+
| O | V | LV |
+---+---+----+
| 1 | 1 | 1 |
| 2 | | 1 |
| 3 | 3 | 3 |
| 4 | | 3 |
| 5 | | 3 |
+---+---+----+
(5 rows)

!ok

# IGNORE NULLS returns NULL when every row in the frame is null.
select o, v,
first_value(v) ignore nulls
over (order by o rows between 1 preceding and 1 preceding) as fv
from (values (1, cast(null as integer)), (2, cast(null as integer)),
(3, 5)) as t(o, v);
+---+---+----+
| O | V | FV |
+---+---+----+
| 1 | | |
| 2 | | |
| 3 | 5 | |
+---+---+----+
(3 rows)

!ok

# RESPECT NULLS (the default) still returns the boundary value, including NULL.
select o, v,
first_value(v) respect nulls over (order by o rows 2 preceding) as fv,
last_value(v) over (order by o rows 2 preceding) as lv
from (values (1, 1), (2, cast(null as integer)), (3, 3)) as t(o, v);
+---+---+----+----+
| O | V | FV | LV |
+---+---+----+----+
| 1 | 1 | 1 | 1 |
| 2 | | 1 | |
| 3 | 3 | 1 | 3 |
+---+---+----+----+
(3 rows)

!ok

# End winagg.iq
Loading