Jackie-Jiang commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4275959098
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -456,6 +465,101 @@ public RelDataType inferReturnType(SqlOperatorBinding
opBinding) {
}
}
+ /// Casts the value of `ARRAY_AGG(value, 'dataType'[, isDistinct])` to the
data type, so that the return type that
+ /// [ArrayOfComponentReturnTypeInference] infers from the value is the type
of the returned array. The function picks
+ /// its accumulator by the data type, not by the type of the value: for
example, `ARRAY_AGG(intCol, 'LONG')` returns
+ /// longs. The cast changes the operand instead of the return type
inference, because Calcite infers the type of an
+ /// aggregate call again after the validation (e.g.
`Aggregate.typeMatchesInferred()`), and it cannot read the value
+ /// of a literal operand then.
+ private static class ArrayAggOperandTypeChecker implements
SqlOperandTypeChecker {
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean
throwOnFailure) {
+ if (callBinding.getOperandCount() < 2 ||
!callBinding.isOperandLiteral(1, false)
+ || !SqlTypeUtil.inCharFamily(callBinding.getOperandType(1))) {
+ // The function fails at runtime without a data type
+ return true;
+ }
+ String dataTypeName = callBinding.getOperandLiteralValue(1,
String.class);
+ if (dataTypeName == null) {
+ return true;
+ }
+ RelDataTypeFactory typeFactory = callBinding.getTypeFactory();
+ RelDataType dataType = toRelDataType(dataTypeName, typeFactory);
+ RelDataType valueType = callBinding.getOperandType(0);
+ // Keep multi-value columns, for which the return type inference uses
the type of the elements
+ if (dataType == null || valueType.getComponentType() != null ||
!needsCast(valueType, dataType)) {
Review Comment:
The scalar correction works, but skipping every array operand leaves the
same mismatch for multivalue inputs. For example, `SELECT ARRAY_AGG(CAST(intMv
AS DECIMAL(10,0) ARRAY), 'INT') FROM tbl`, with intMv=[1,2], previously planned
INT_ARRAY and matched the accumulator's IntArrayList. The new decimal mapping
makes the cast and inferred aggregate type LONG_ARRAY, while this early return
leaves the explicit 'INT' accumulator unchanged. TypeUtils.convert(...,
LONG_ARRAY) cannot convert IntArrayList, so output conversion asserts or
serialization fails. DECIMAL(19,0) ARRAY with 'LONG' has the analogous
LONG_ARRAY-to-DOUBLE_ARRAY mismatch. Please normalize the component type to the
declared datatype while preserving the array shape, and add coverage alongside
the scalar cases.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java:
##########
@@ -1137,6 +1139,38 @@ private static ColumnDataType resolveDecimal(RelDataType
relDataType, boolean is
}
}
+ /// Returns whether a CAST of a value of the operand type to the target type
converts the value to LONG first.
+ ///
+ /// [#resolveDecimal] gives DECIMAL(p, 0) with 19 <= p <= 38 the type
BIG_DECIMAL, which keeps the fraction of a
+ /// value. A CAST to these types from a type that is not an integer type
(e.g. DOUBLE) converts the value through
+ /// LONG, as it did when the planner gave these types the type LONG, so that
the result is an integer. As before, such
+ /// a CAST does not hold values beyond the LONG range: a DOUBLE saturates, a
BIG_DECIMAL wraps around, and a STRING
+ /// fails to parse. A CAST to DECIMAL(p, 0) with p > 38 keeps the fraction,
as before.
+ public static boolean isCastThroughLong(RelDataType targetType, RelDataType
operandType) {
+ if (targetType.getSqlTypeName() != SqlTypeName.DECIMAL ||
targetType.getScale() != 0) {
+ return false;
+ }
+ int precision = targetType.getPrecision();
+ if (precision <= 18 || precision > 38) {
+ return false;
+ }
+ switch (operandType.getSqlTypeName()) {
+ case TINYINT:
+ case SMALLINT:
+ case INTEGER:
+ case BIGINT:
+ case UTINYINT:
+ case USMALLINT:
+ case UINTEGER:
+ case UBIGINT:
+ return false;
+ case DECIMAL:
+ return operandType.getScale() != 0;
Review Comment:
A scale-zero DECIMAL operand is not always integer-valued under the behavior
intentionally retained here. With doubleCol=1.2, `CAST(CAST(doubleCol AS
DECIMAL(40,0)) AS DECIMAL(19,0))` still returns 1.2: the inner p>38 cast keeps
the fraction, and this branch skips the outer truncation because the operand's
declared scale is zero. Before this PR the outer LONG conversion returned 1.
Calcite retains these nested casts because DOUBLE-to-DECIMAL is not a lossless
cast, so this is reachable at runtime. Please handle this case without wrapping
legitimate large integer operands, and add a nested-cast regression case.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -132,12 +133,21 @@ public Integer apply(List<Object> row) {
if (result == null) {
return null;
}
- if (firstResult.equals(result)) {
+ if (valuesEqual(firstResult, result)) {
return _isNotIn ? 0 : 1;
}
}
return _isNotIn ? 1 : 0;
}
+
+ /// Compares `BIG_DECIMAL` values by number, like a comparison does and
like the single-stage engine does for IN.
+ /// `BigDecimal.equals()` also compares the scale, so 1.0 (e.g. a SUM,
which is a DOUBLE) does not equal 1.
+ private static boolean valuesEqual(Object value1, Object value2) {
+ if (value1 instanceof BigDecimal && value2 instanceof BigDecimal) {
+ return ((BigDecimal) value1).compareTo((BigDecimal) value2) == 0;
Review Comment:
The same-version fix works, but the documented rolling-upgrade limitation is
a newly incorrect result for queries that worked before this PR. A new broker
with an older server still returns false for `SELECT SUM(CAST(intCol AS
DECIMAL(19,0))) IN (CAST(1 AS DECIMAL(19,0)), CAST(2 AS DECIMAL(19,0))) FROM
tbl WHERE intCol = 1`: the new plan uses BIG_DECIMAL, the aggregate becomes
BigDecimal("1.0"), and the older worker compares it to scale-zero literals with
equals. HAVING can therefore lose rows during an upgrade.
RexExpressionUtils.handleSearch still emits IN/NOT_IN for pure point sets; the
older-server safeguard in convertRanges covers only other range sets. Please
extend broker-side comparison lowering to decimal point/complement predicates
too, or gate the changed mapping until workers support numeric membership.
--
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]
