xiangfu0 commented on code in PR #19788:
URL: https://github.com/apache/pinot/pull/19788#discussion_r4278301496
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotEvaluateLiteralRule.java:
##########
@@ -251,6 +264,112 @@ private static RexNode
evaluateLiteralOnlyFunction(RexCall rexCall, RexBuilder r
}
}
+ /// Returns whether the call is `datetime + interval` (Calcite's
`DATETIME_PLUS`) or `datetime - interval`
+ /// (`MINUS_DATE`) with a `TIMESTAMP` or `DATE` result. Calcite puts the
interval second for both.
+ private static boolean isDatetimeIntervalArithmetic(RexCall rexCall) {
+ SqlKind kind = rexCall.getKind();
+ SqlTypeName resultType = rexCall.getType().getSqlTypeName();
+ List<RexNode> operands = rexCall.getOperands();
+ return (kind == SqlKind.PLUS || kind == SqlKind.MINUS) && (resultType ==
SqlTypeName.TIMESTAMP
+ || resultType == SqlTypeName.DATE) && operands.size() == 2 &&
SqlTypeUtil.isInterval(operands.get(1).getType());
+ }
+
+ /// Evaluates `datetime + interval` or `datetime - interval` with the same
semantics as Calcite. It cannot use Pinot's
+ /// `plus` / `minus` functions, because Calcite stores a day-time interval
as milliseconds but a year-month interval
+ /// as months: they would add `INTERVAL '1' MONTH` as 1 millisecond. Months
are added with the same calendar
+ /// arithmetic as `TIMESTAMPADD(MONTH, ...)`, so `2019-01-31 + INTERVAL '1'
MONTH` is `2019-02-28`. Like Calcite, only
+ /// the whole days of a day-time interval are added to a `DATE`.
+ private static RexNode evaluateDatetimeIntervalArithmetic(RexCall rexCall,
RexBuilder rexBuilder) {
+ RexLiteral datetime = (RexLiteral) rexCall.getOperands().get(0);
+ RexLiteral interval = (RexLiteral) rexCall.getOperands().get(1);
+ RelDataType resultType = rexCall.getType();
+ if (datetime.isNull() || interval.isNull()) {
+ return rexBuilder.makeNullLiteral(resultType);
+ }
+ Object resultValue;
+ try {
+ long millis =
Objects.requireNonNull(datetime.getValueAs(Calendar.class)).getTimeInMillis();
+ // Number of months for a year-month interval, number of milliseconds
for a day-time interval
+ long intervalValue =
Objects.requireNonNull(interval.getValueAs(BigDecimal.class)).longValueExact();
+ if (rexCall.getKind() == SqlKind.MINUS) {
+ intervalValue = Math.negateExact(intervalValue);
+ }
+ boolean isDate = resultType.getSqlTypeName() == SqlTypeName.DATE;
+ long resultMillis;
+ if
(SqlTypeName.YEAR_INTERVAL_TYPES.contains(interval.getType().getSqlTypeName()))
{
+ resultMillis = DateTimeFunctions.timestampAdd("MONTH", intervalValue,
millis);
+ } else {
+ if (isDate) {
+ intervalValue = intervalValue / DateTimeUtils.MILLIS_PER_DAY *
DateTimeUtils.MILLIS_PER_DAY;
+ }
+ resultMillis = Math.addExact(millis, intervalValue);
+ }
+ resultValue = isDate ? DateString.fromDaysSinceEpoch(
+ Math.toIntExact(Math.floorDiv(resultMillis,
DateTimeUtils.MILLIS_PER_DAY))) : resultMillis;
+ } catch (Exception e) {
+ throw new SqlCompilationException(
+ "Caught exception while evaluating: " + rexCall + ": " +
e.getMessage(), e);
+ }
+ return rexBuilder.makeLiteral(resultValue, resultType, false);
+ }
+
+ /// Returns whether the call is `+`, `-`, `*` or `/` with an interval result
and only interval or numeric operands,
+ /// e.g. `INTERVAL '1' MONTH * 2`.
+ private static boolean isIntervalArithmetic(RexCall rexCall) {
+ SqlKind kind = rexCall.getKind();
+ List<RexNode> operands = rexCall.getOperands();
+ return (kind == SqlKind.PLUS || kind == SqlKind.MINUS || kind ==
SqlKind.TIMES || kind == SqlKind.DIVIDE)
+ && SqlTypeUtil.isInterval(rexCall.getType()) && operands.size() == 2
&& operands.stream()
+ .allMatch(operand -> SqlTypeUtil.isInterval(operand.getType()) ||
SqlTypeUtil.isNumeric(operand.getType()));
+ }
+
+ /// Evaluates interval arithmetic exactly. Pinot's `times` / `divide`
functions compute in DOUBLE, so
+ /// `INTERVAL '100' MONTH * 0.29` would be 28.999999999999996 months instead
of 29, and `INTERVAL '1' DAY * 0.7` would
+ /// be 1 millisecond short.
+ private static RexNode evaluateIntervalArithmetic(RexCall rexCall,
RexBuilder rexBuilder) {
+ RexLiteral left = (RexLiteral) rexCall.getOperands().get(0);
+ RexLiteral right = (RexLiteral) rexCall.getOperands().get(1);
+ RelDataType resultType = rexCall.getType();
+ if (left.isNull() || right.isNull()) {
+ return rexBuilder.makeNullLiteral(resultType);
+ }
+ BigDecimal resultValue;
+ try {
+ // An interval literal holds its number of months or milliseconds
+ BigDecimal leftValue =
Objects.requireNonNull(left.getValueAs(BigDecimal.class));
+ BigDecimal rightValue =
Objects.requireNonNull(right.getValueAs(BigDecimal.class));
+ switch (rexCall.getKind()) {
+ case PLUS:
+ resultValue = leftValue.add(rightValue);
+ break;
+ case MINUS:
+ resultValue = leftValue.subtract(rightValue);
+ break;
+ case TIMES:
+ resultValue = leftValue.multiply(rightValue);
+ break;
+ default:
+ resultValue = leftValue.divide(rightValue, MathContext.DECIMAL128);
+ break;
+ }
+ resultValue = toIntervalValue(resultValue, resultType);
+ } catch (Exception e) {
+ throw new SqlCompilationException(
+ "Caught exception while evaluating: " + rexCall + ": " +
e.getMessage(), e);
+ }
+ return rexBuilder.makeLiteral(resultValue, resultType, false);
+ }
+
+ /// Converts a number to the value that Calcite stores for an interval: a
whole number of months for a year-month
+ /// interval, and milliseconds for a day-time interval (like Calcite,
fractions of a millisecond are dropped).
+ private static BigDecimal toIntervalValue(BigDecimal value, RelDataType
intervalType) {
+ if
(SqlTypeName.YEAR_INTERVAL_TYPES.contains(intervalType.getSqlTypeName())) {
Review Comment:
Minor / fail-safe divergence from Calcite runtime, not blocking. Calcite
stores a year-month interval as an integer month count, so `INTERVAL '1' MONTH
/ 2` silently truncates to `INTERVAL '0' MONTH` at runtime rather than
erroring. Here the fold throws instead. That's arguably better (hard error vs
silent 0), but it does mean the folded form rejects a query the runtime path
would accept. Consider either truncating with `RoundingMode.DOWN` to match
Calcite, or adding a short comment noting this is an intentional
stricter-than-Calcite rejection.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotEvaluateLiteralRule.java:
##########
@@ -310,6 +429,13 @@ private static Object convertResultValue(@Nullable Object
resultValue, RelDataTy
if (relDataType.getSqlTypeName() == SqlTypeName.DECIMAL) {
return new BigDecimal(resultValue.toString());
}
+ // Other functions with an interval result, e.g. ABS(INTERVAL '-1' MONTH).
Interval arithmetic is evaluated exactly
+ // in evaluateIntervalArithmetic().
+ if (SqlTypeUtil.isInterval(relDataType)) {
+ double value = ((Number) resultValue).doubleValue();
Review Comment:
Minor: this generic interval branch routes the value through `double` before
`toIntervalValue`, which gives up the exactness the arithmetic path works hard
to preserve with BigDecimal. In practice the only callers here (e.g.
`ABS(INTERVAL ...)`) return exact integers so it's fine, but if a `Number`
subtype that's already a `BigDecimal` reaches this, converting via
`doubleValue()` is a small, avoidable precision hazard — you could branch on
`resultValue instanceof BigDecimal` and skip the double.
--
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]
