yashmayya commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4253696300
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java:
##########
@@ -1115,9 +1114,12 @@ private static ColumnDataType resolveDecimal(RelDataType
relDataType, boolean is
int precision = relDataType.getPrecision();
int scale = relDataType.getScale();
if (scale == 0) {
- if (precision <= 10) {
+ // Pick the type by the precision, so that it holds every value of the
DECIMAL type: INT holds every number with
+ // up to 9 digits, and LONG every number with up to 18 digits. For
example, Calcite gives an integer literal
+ // beyond the BIGINT range the type DECIMAL(p, 0) with p >= 19, which
LONG would wrap around.
+ if (precision <= 9) {
return isArray ? ColumnDataType.INT_ARRAY : ColumnDataType.INT;
- } else if (precision <= 38) {
+ } else if (precision <= 18) {
Review Comment:
Thanks, confirmed. `ARRAY_AGG(CAST(longCol AS DECIMAL(19, 0)), 'LONG')` and
the `DECIMAL(10, 0)` / `'INT'` variant fail with this change and work on
master. The root cause is older: `ARRAY_AGG(intCol, 'LONG')` and
`ARRAY_AGG(doubleCol, 'FLOAT')` already fail on master. The plan type follows
the value, but the accumulator follows the data type argument.
I could not read the literal in the return type inference. Calcite infers
aggregate types again through `AggCallBinding` (for example in the
`typeMatchesInferred` assertion of the `Aggregate` constructor), and that
binding cannot read literal operands. So `ArrayAggOperandTypeChecker` now casts
the value to the explicit data type during validation, when the types differ.
The existing inference then returns `ARRAY<data type>` in all places. The fix
is in the planner only, so it covers both planners and older servers.
One visible change: when the value has another type that already worked, the
result now has the data type, like the single-stage engine. For example,
`ARRAY_AGG(longCol, 'TIMESTAMP')` returns timestamps instead of longs. I added
this to the PR description.
Tests cover the two DECIMAL cases, the two cases that fail on master, the
values of `'FLOAT'` and `'BIG_DECIMAL'` arrays, and a plan that shows the cast.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java:
##########
@@ -1115,9 +1114,12 @@ private static ColumnDataType resolveDecimal(RelDataType
relDataType, boolean is
int precision = relDataType.getPrecision();
int scale = relDataType.getScale();
if (scale == 0) {
- if (precision <= 10) {
+ // Pick the type by the precision, so that it holds every value of the
DECIMAL type: INT holds every number with
+ // up to 9 digits, and LONG every number with up to 18 digits. For
example, Calcite gives an integer literal
+ // beyond the BIGINT range the type DECIMAL(p, 0) with p >= 19, which
LONG would wrap around.
+ if (precision <= 9) {
return isArray ? ColumnDataType.INT_ARRAY : ColumnDataType.INT;
- } else if (precision <= 38) {
+ } else if (precision <= 18) {
Review Comment:
Thanks, confirmed. This query returns false with this change and true on
master. The same problem already exists on master for BIG_DECIMAL columns:
`SUM(bigDecimalCol) IN (1, 2)` returns false when the sum is 1.
`FilterOperand.In` now compares BigDecimal values with `compareTo`, like the
comparison operators and like the single-stage `IN`, which uses a `TreeSet` for
this reason. This also fixes the BIG_DECIMAL column case. Tests cover `IN` in a
projection and in `HAVING`, and `NOT IN`, for `DECIMAL(19, 0)` and `DECIMAL(40,
0)`.
During a rolling upgrade, servers that are not upgraded yet still compare
with `equals`.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/v2/PRelToPlanNodeConverter.java:
##########
@@ -375,9 +374,12 @@ private static ColumnDataType resolveDecimal(RelDataType
relDataType, boolean is
int precision = relDataType.getPrecision();
int scale = relDataType.getScale();
if (scale == 0) {
- if (precision <= 10) {
+ // Pick the type by the precision, so that it holds every value of the
DECIMAL type: INT holds every number with
+ // up to 9 digits, and LONG every number with up to 18 digits. For
example, Calcite gives an integer literal
+ // beyond the BIGINT range the type DECIMAL(p, 0) with p >= 19, which
LONG would wrap around.
+ if (precision <= 9) {
return isArray ? ColumnDataType.INT_ARRAY : ColumnDataType.INT;
- } else if (precision <= 38) {
+ } else if (precision <= 18) {
Review Comment:
Thanks, confirmed. `CAST(doubleCol AS DECIMAL(19, 0))` returns 1.2 with this
change and 1 on master, in the leaf stage and in intermediate stages. Master
already keeps the fraction for `DECIMAL(p, 0)` with p > 38.
To round a BIG_DECIMAL to the requested scale exactly, servers need more
than the target type name that they get now, and older servers do not support
it. So a CAST to `DECIMAL(p, 0)` with 19 <= p <= 38 from a non-integer type now
converts through LONG first, as on master. It drops the fraction (truncation,
like the LONG cast) and returns a BIG_DECIMAL with scale 0. Integer inputs
still cast directly, so their values beyond the LONG range stay exact. For
non-integer inputs, the old LONG range limit stays: a DOUBLE saturates and a
BIG_DECIMAL wraps, as on master. The planning-time fold in
`PinotEvaluateLiteralRule` does the same.
Tests cover DOUBLE and BIG_DECIMAL inputs in the leaf stage, in an
intermediate stage, and in a call that the planner evaluates.
--
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]
