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/19 21:24:01 UTC
[PR] Revert "Map DECIMAL(p, 0) to a type that holds all its values in the multi-stage engine (#19790)" [pinot]
yashmayya opened a new pull request, #19949:
URL: https://github.com/apache/pinot/pull/19949
This reverts #19790. That PR maps `DECIMAL(p, 0)` with `19 <= p <= 38` to `BIG_DECIMAL` (was `LONG`) in the multi-stage engine. The change reaches more queries than planned, and it causes wrong results. #19790 is only on master (`1.7.0-SNAPSHOT`), so this revert brings back the released behavior.
## Problem
Common expressions get the type `DECIMAL(19, 0)`:
1. Calcite gives a `DECIMAL` type to `MOD(doubleCol, 100)`, `longCol * 1.5` and `ts / 1000.0`. Pinot's `TypeSystem` caps these derived types at `DECIMAL(19, 1)`.
2. Calcite gives `CEIL` and `FLOOR` of `DECIMAL(p, s)` the type `DECIMAL(p, 0)`. Here, that is `DECIMAL(19, 0)`.
3. With #19790, `DECIMAL(19, 0)` maps to `BIG_DECIMAL`.
As a result, `FLOOR(ts / 1000.0)`, `FLOOR(x * 0.001)` and `CEIL(MOD(doubleCol, 100))` return `BIG_DECIMAL` (was `LONG`). The broker sends these values as strings with a fraction, for example `"4.0"` instead of `4`. `SUM`, `MIN`, `MAX`, `AVG`, `GROUP BY`, `DISTINCT`, window functions, `UNION` and `CASE` carry the new type.
Servers compute `ceil` and `floor` in `double`. The value `3.0` becomes `BigDecimal("3.0")`, but an `INT` column cast to `BIG_DECIMAL` becomes `BigDecimal("3")`. Hash joins and set operations compare keys with `BigDecimal.equals`, which also compares the scale. Results for a table with a row where `d = 3.7` and `intCol = 3`:
| Query | Before #19790 | After #19790 |
|---|---|---|
| `t1 JOIN t2 ON FLOOR(MOD(t1.d, 100)) = t2.intCol` | 1 row | 0 rows |
| `WHERE intCol IN (SELECT FLOOR(MOD(d, 100)) ...)`, physical optimizer | 1 row | 0 rows |
| `WHERE intCol NOT IN (SELECT FLOOR(MOD(d, 100)) ...)` | row with `intCol = 3` excluded | row with `intCol = 3` returned |
| `SELECT FLOOR(MOD(d, 100)) ... INTERSECT SELECT intCol ...` | `3` | empty |
| `SELECT FLOOR(MOD(d, 100)) ... EXCEPT SELECT intCol ...` | `3` removed | `3.0` kept |
| `SELECT FLOOR(MOD(d, 100)) ... UNION SELECT intCol ...` | `3` once | `3` and `3.0` |
## Why a revert and not a fix forward
- The bugs that #19790 fixes need an integer literal beyond the `BIGINT` range, or a `CAST` to `DECIMAL(10..38, 0)`. These are rare. The shapes above are common.
- A fix forward needs two larger changes. `CEIL` and `FLOOR` need a smaller precision, but Calcite's `SqlFloorFunction` blocks a replacement through the operator table. Joins and set operations need `BigDecimal` keys that do not depend on the scale.
## What comes back with the revert
- The wrap of `DECIMAL(p, 0)` literals beyond the `INT` and `LONG` range, as listed in #19790. A later PR can fix this at the literal, without a change to the type mapping for expressions.
- `SUM(bigDecimalCol) IN (1, 2)` and `ARRAY_AGG(intCol, 'LONG')` fail again, as before #19790. These fixes do not need the new type mapping, so they can come back as separate PRs.
## Tests
The revert applies to master with no conflicts. These tests pass locally: `RelToPlanNodeConverterTest`, `RexExpressionUtilsTest`, `PRelToPlanNodeConverterTest`, `ResourceBasedQueryPlansTest`, `QueryCompilationTest`, `FilterOperatorTest` and `ResourceBasedQueriesTest`.
--
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] Revert "Map DECIMAL(p, 0) to a type that holds all its values in the multi-stage engine (#19790)" [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #19949:
URL: https://github.com/apache/pinot/pull/19949
--
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] Revert "Map DECIMAL(p, 0) to a type that holds all its values in the multi-stage engine (#19790)" [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19949:
URL: https://github.com/apache/pinot/pull/19949#issuecomment-6279116569
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19949?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `84.61538%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 58.94%. Comparing base ([`179993a`](https://app.codecov.io/gh/apache/pinot/commit/179993a61b12d64300d333fdd72cf7529d7aa2e8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`47212ae`](https://app.codecov.io/gh/apache/pinot/commit/47212aec62663a793c7be38ddae43d0ea8fcfc67?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19949?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...y/planner/physical/v2/PRelToPlanNodeConverter.java](https://app.codecov.io/gh/apache/pinot/pull/19949?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fphysical%2Fv2%2FPRelToPlanNodeConverter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9waHlzaWNhbC92Mi9QUmVsVG9QbGFuTm9kZUNvbnZlcnRlci5qYXZh) | 0.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19949?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (179993a) and HEAD (47212ae). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (179993a) | HEAD (47212ae) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration1|2|0|
>|lane-b|2|0|
>|unittests2|1|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19949 +/- ##
=============================================
- Coverage 69.53% 58.94% -10.59%
+ Complexity 1491 1 -1490
=============================================
Files 3546 2729 -817
Lines 234256 171747 -62509
Branches 37521 28305 -9216
=============================================
- Hits 162883 101236 -61647
- Misses 58811 61960 +3149
+ Partials 12562 8551 -4011
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19949/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/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19949/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/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.94% <84.61%> (-10.59%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.94% <84.61%> (-10.59%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.94% <84.61%> (-10.59%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19949/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.94% <84.61%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19949/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/19949?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] Revert "Map DECIMAL(p, 0) to a type that holds all its values in the multi-stage engine (#19790)" [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19949:
URL: https://github.com/apache/pinot/pull/19949#issuecomment-6278666852
Documentation follow-up merged: https://github.com/pinot-contrib/pinot-docs/pull/1121 — removes the upcoming-release DECIMAL note from docs #1118 so the docs no longer advertise the reverted #19790 changes.
--
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