vlsi commented on code in PR #5304:
URL: https://github.com/apache/calcite/pull/5304#discussion_r4189675122
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -5209,6 +5213,26 @@ private ImmutableList<RexFieldCollation>
simplifySortKeys(
}
return newSortKeys.build();
}
+
+ /** Replaces {@code LAG(x, 0)} and {@code LEAD(x, 0)}, with or without a
+ * default, by {@code x}. Returns null if the call cannot be replaced. */
+ private @Nullable RexNode simplifyZeroOffset(RelDataType type) {
+ if (op.getKind() != SqlKind.LAG && op.getKind() != SqlKind.LEAD) {
+ return null;
+ }
+ // Check if the offset is 0; (the default offset is 1)
+ if (operands.size() < 2
+ || !RexSimplify.checkLiteralValue(operands.get(1), BigDecimal.ZERO))
{
+ return null;
+ }
+ final RexNode value = operands.get(0);
+ // The result must be nullable
Review Comment:
`SqlLeadLagAggFunction` infers a NOT NULL type only when every operand is
NOT NULL (CALCITE-7834), so for that operator `!type.isNullable() &&
value.getType().isNullable()` is never true and no test reaches this branch.
Remove the check or turn it into an assertion. The comment "The result must be
nullable" also misdescribes it: the check skips the rewrite when the call type
is NOT NULL and `x` is nullable.
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -5142,6 +5142,10 @@ private OverCall
orderBy_(ImmutableList<RexFieldCollation> sortKeys) {
}
};
final RelDataType type = op.inferReturnType(bind);
+ final RexNode currentRowValue = simplifyZeroOffset(type);
Review Comment:
Without an alias, `OverCall.toRex()` used to always return a `RexOver`. Now
it can return the value operand itself, or a `CAST` of it. The Javadoc of
`OverCall.toRex()` doesn't mention this, and code outside Calcite that casts
the result to `RexOver` will throw `ClassCastException`. Document it, and add a
`RelBuilderTest` case that builds `LAG(x, 0)` through
`aggregateCall(...).over()` and checks the result.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -439,7 +439,7 @@ private static int findLiteralIndex(List<RexNode> operands,
BigDecimal value) {
}
/** Check whether the operand is a BigDecimal literal of the specified
value. */
- private static boolean checkLiteralValue(RexNode operand, BigDecimal value) {
+ public static boolean checkLiteralValue(RexNode operand, BigDecimal value) {
Review Comment:
This makes `checkLiteralValue` part of the public API of `RexSimplify` for a
single caller in `RelBuilder`. Keep it private and add a small helper in
`RelBuilder`, or move it to `RexLiteral` with a Javadoc written as a public
contract.
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -5209,6 +5213,26 @@ private ImmutableList<RexFieldCollation>
simplifySortKeys(
}
return newSortKeys.build();
}
+
+ /** Replaces {@code LAG(x, 0)} and {@code LEAD(x, 0)}, with or without a
+ * default, by {@code x}. Returns null if the call cannot be replaced. */
+ private @Nullable RexNode simplifyZeroOffset(RelDataType type) {
+ if (op.getKind() != SqlKind.LAG && op.getKind() != SqlKind.LEAD) {
+ return null;
+ }
+ // Check if the offset is 0; (the default offset is 1)
+ if (operands.size() < 2
+ || !RexSimplify.checkLiteralValue(operands.get(1), BigDecimal.ZERO))
{
+ return null;
+ }
+ final RexNode value = operands.get(0);
+ // The result must be nullable
+ if (!type.isNullable() && value.getType().isNullable()) {
+ return null;
+ }
+ // Preserve the type of the call
+ return getRexBuilder().ensureType(pos, type, value, false);
Review Comment:
No row covers a default that widens the type. For `lag(i, 0, 1.5)` with `i
INTEGER`, the call type is `DECIMAL(11, 1)` and the inserted `CAST` returns
`10.0` for `i = 10`. `lag(i, 1-1, 1.5)` has the same type and returns `10`,
because `LeadLagImplementor` does not rescale the value to the result type. Add
`lag(i, 0, 1.5)` to `winagg.iq`, and file the rescaling in `LeadLagImplementor`
as a separate issue.
--
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]
