You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/10/07 23:21:01 UTC
[PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
yashmayya opened a new pull request, #19788:
URL: https://github.com/apache/pinot/pull/19788
## Problem
The multi-stage engine folds `datetime +/- INTERVAL` literals through the `plus` / `minus` scalar functions and passes the raw Calcite interval value. Calcite stores a day-time interval as milliseconds, but a year-month interval as a number of **months**. So `INTERVAL '1' MONTH` is added as 1 ms. The result is silently wrong:
| Query | Before | After |
|---|---|---|
| `TIMESTAMP '2019-01-01 00:00:00' + INTERVAL '1' MONTH` | `2019-01-01 00:00:00` | `2019-02-01 00:00:00` |
| `TIMESTAMP '2019-03-31 00:00:00' - INTERVAL '1' MONTH` | `2019-03-30 23:59:59` | `2019-02-28 00:00:00` |
| `WHERE ts > TIMESTAMP '2019-03-01 00:00:00' - INTERVAL '1' MONTH` | `> 2019-02-28 23:59:59` | `> 2019-02-01 00:00:00` |
| `DATE '2019-01-01' + INTERVAL '1' DAY` | `ClassCastException` | `2019-01-02` |
| `TIMESTAMP '...' + INTERVAL '1' MONTH * 2` | `AssertionError` (adds 2 ms without `-ea`) | +2 months |
Day-time intervals (`DAY`, `HOUR`, ...) were already correct and do not change.
## Fix
`PinotEvaluateLiteralRule` now evaluates `DATETIME_PLUS` / `MINUS_DATE` itself, with the same semantics as Calcite:
- Year-month interval: add months with the same calendar arithmetic as `TIMESTAMPADD(MONTH, ...)`, so `2019-01-31 + INTERVAL '1' MONTH` is `2019-02-28`.
- Day-time interval: add milliseconds, as before.
- `DATE` result: add only the whole days of a day-time interval, and build a `DATE` literal.
Calls that return an interval (for example `INTERVAL '1' MONTH * 2`) now fold to a correct interval literal. A fractional number of months (`INTERVAL '1' MONTH / 2`) fails with a clear error.
Not in scope: `ts_col + INTERVAL ...` with a column operand still fails at planning (`Unsupported ColumnDataType: OBJECT`) for all interval units, as before.
## Tests
Added cases to `LiteralEvaluationPlans.json` for YEAR, MONTH, YEAR TO MONTH, DAY, HOUR, DAY TO SECOND, end of month, negative intervals, WHERE clauses, DATE results and multiplied intervals. 16 of them fail without the fix.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #19788:
URL: https://github.com/apache/pinot/pull/19788
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #19788:
URL: https://github.com/apache/pinot/pull/19788#issuecomment-6157346648
@Jackie-Jiang Good catch, thanks. Fixed in c1f5e1eaf9.
`+ - * /` calls that return an interval are now evaluated exactly with `BigDecimal` (the literal values Calcite stores), not through the DOUBLE `times` / `divide` functions. The whole-month check then runs on the exact value. `INTERVAL '100' MONTH * 0.29` is now 29 months and `* 1.11` is 111 months. Both are regression cases now.
The same DOUBLE rounding also made `INTERVAL '1' DAY * 0.7` silently 1 ms short (shown as `16:47:59` instead of `16:48:00` with a seconds-precision literal). That is fixed too, with a test. Other functions that return an interval (for example `ABS(INTERVAL '-1' MONTH)`) still go through the DOUBLE path, with the same check.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19788:
URL: https://github.com/apache/pinot/pull/19788#issuecomment-6049363265
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19788?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`4d93fce`](https://app.codecov.io/gh/apache/pinot/commit/4d93fce3399c7796016089cdae5bd7196b7982fe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3484b7c`](https://app.codecov.io/gh/apache/pinot/commit/3484b7c553ddf9549bd54e7248689b6177afbdde?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (4d93fce) and HEAD (3484b7c). Click for more details.
>
> <details><summary>HEAD has 18 uploads less than BASE</summary>
>
>| Flag | BASE (4d93fce) | HEAD (3484b7c) |
>|------|------|------|
>|unittests1|1|0|
>|unittests|2|0|
>|java-25|6|2|
>|temurin|6|2|
>|lane-a|2|1|
>|integration|4|2|
>|integration1|2|0|
>|lane-b|2|1|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19788 +/- ##
=============================================
- Coverage 68.55% 0.00% -68.56%
=============================================
Files 3522 3 -3519
Lines 229564 6 -229558
Branches 36457 0 -36457
=============================================
- Hits 157383 0 -157383
+ Misses 59851 6 -59845
+ Partials 12330 0 -12330
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-68.56%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-68.56%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19788/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19788?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #19788:
URL: https://github.com/apache/pinot/pull/19788#issuecomment-6200059977
@xiangfu0 Thanks for the review. Added two null cases in ff10534427.
`CAST(NULL AS INTERVAL MONTH)` fails before this rule, in validation (`BytesCastVisitor`, an unrelated issue that was already there). Calcite also folds most other null forms before this rule runs. So the tests use a Pinot function that returns null:
- `fromDateTime(CAST(NULL AS VARCHAR), 'yyyy-MM-dd') + INTERVAL '1' MONTH` (null datetime)
- `TIMESTAMP '2019-01-01 00:00:00' + INTERVAL '1' MONTH * toEpochDays(fromDateTime(CAST(NULL AS VARCHAR), 'yyyy-MM-dd'))` (null interval)
I checked that both cases reach the null branches.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19788:
URL: https://github.com/apache/pinot/pull/19788#discussion_r4287148077
##########
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:
Kept the rejection on purpose and added a comment in ff10534427 that says it is stricter than Calcite. A silent `0` months is a wrong result, so an error is safer. Also, Pinot has no runtime path for intervals: an interval that is not folded fails at planning (`Unsupported ColumnDataType: OBJECT`). So this does not reject a query that Pinot could otherwise run.
##########
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:
Done in ff10534427: a `BigDecimal` result now goes to `toIntervalValue` directly, without the `double` conversion.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19788:
URL: https://github.com/apache/pinot/pull/19788#discussion_r4265398018
##########
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));
Review Comment:
Non-blocking precision edge case: for a DOUBLE literal, Calcite's `getValueAs(BigDecimal.class)` uses `MathContext.DECIMAL64`, so it rounds `1.9999999999999998` to `2` before the interval arithmetic runs.
```sql
SELECT TIMESTAMP '2019-01-01 00:00:00.000'
+ INTERVAL '1' SECOND * 1.9999999999999998E0
FROM a
```
This now folds to `2019-01-01 00:00:02.000`; both the original base and the previous PR revision fold to `2019-01-01 00:00:01.999`. The same issue occurs with `CAST(1.9999999999999998 AS DOUBLE)`. Please preserve the numeric operand's precision when converting it to BigDecimal and add a regression case around this millisecond boundary. Confirmed with isolated planner probes against all three versions.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix MSE constant folding of year-month INTERVAL arithmetic [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org