Jackie-Jiang commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4253103845
##########
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:
Widening the runtime cast to BIG_DECIMAL also drops the previously enforced
zero scale. For a DOUBLE column containing 1.2, `SELECT CAST(doubleCol AS
DECIMAL(19,0)) FROM tbl` now preserves 1.2 instead of producing an
integer-valued decimal (the previous LONG conversion returned 1).
RexExpressionUtils.handleCast passes only the mapped type name, without
precision or scale; CastTransformFunction and the intermediate scalar cast then
convert to BigDecimal without removing the fractional part. Carry and enforce
the requested scale when performing these casts, and add a fractional-input
regression test. The logical converter has the same issue.
##########
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:
The new BIG_DECIMAL aggregate results expose scale-sensitive IN equality.
With one input row x=1, consider `SELECT SUM(CAST(x AS DECIMAL(19,0))) IN
(CAST(1 AS DECIMAL(19,0)), CAST(2 AS DECIMAL(19,0))) FROM tbl`. SUM over
DECIMAL still uses SumAggregationFunction, which returns Double.
MultistageAggregationExecutor then converts 1.0 through TypeUtils to
BigDecimal("1.0"), while the IN literals have scale zero. FilterOperand.In
compares them with equals, so this path rejects the numerically equal value;
before this change the aggregate and literals were LONGs and matched. Please
use numeric decimal equality (or consistent normalization) and cover this
aggregate-to-IN path. The physical converter is affected as well.
##########
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:
Preserve ARRAY_AGG's explicit output type when widening this mapping. For
example, `SELECT ARRAY_AGG(CAST(longCol AS DECIMAL(19,0)), 'LONG') FROM tbl`
previously had a LONG_ARRAY plan, matching the LongArrayList returned by
ArrayAggLongFunction. This change makes the inferred result DOUBLE_ARRAY, while
the explicit 'LONG' argument still selects the same accumulator.
MultistageAggregationExecutor passes that result to TypeUtils.convertRow, whose
DOUBLE_ARRAY branch cannot convert LongArrayList: it asserts or passes an
incompatible object to DataBlockBuilder's double[] cast. The analogous
DECIMAL(10,0) / 'INT' query changes INT_ARRAY to LONG_ARRAY and has the same
mismatch. ARRAY_AGG's return-type inference should honor its explicit datatype
argument, with regression coverage for these cases. This also applies to the
physical converter.
--
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]
