xiangfu0 commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4278314591
##########
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) {
Review Comment:
Nice fix. One defensive observation: `valuesEqual` only does the numeric
`compareTo` when *both* sides are `BigDecimal`, and `In` (unlike `Predicate`)
does no type unification. Today this is safe because the planner coerces both
IN sides to the same ColumnDataType, so a BIG_DECIMAL IN always gets BigDecimal
on both sides. But if that coercion ever changes, the `.equals` fallback would
silently return false across different boxed numeric types. Consider comparing
via `Number`/`compareTo` whenever both are numeric, so correctness doesn't rely
solely on the planner. Non-blocking.
##########
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)) {
+ return true;
+ }
+ RelDataType castType = typeFactory.createTypeWithNullability(dataType,
valueType.isNullable());
+ SqlCall call = callBinding.getCall();
+ SqlNode cast = SqlStdOperatorTable.CAST.createCall(SqlParserPos.ZERO,
call.operand(0),
+
SqlTypeUtil.convertTypeToSpec(castType).withNullable(castType.isNullable()));
+ call.setOperand(0, cast);
+ callBinding.getValidator().setValidatedNodeType(cast, castType);
+ return true;
+ }
+
+ /// Returns the SQL type of a Pinot data type name, or `null` when it is
not a data type that ARRAY_AGG supports.
+ @Nullable
+ private static RelDataType toRelDataType(String dataTypeName,
RelDataTypeFactory typeFactory) {
+ switch (dataTypeName.toUpperCase()) {
+ case "INT":
+ return typeFactory.createSqlType(SqlTypeName.INTEGER);
+ case "LONG":
+ return typeFactory.createSqlType(SqlTypeName.BIGINT);
+ case "FLOAT":
+ return typeFactory.createSqlType(SqlTypeName.REAL);
+ case "DOUBLE":
+ return typeFactory.createSqlType(SqlTypeName.DOUBLE);
+ case "BIG_DECIMAL":
+ return typeFactory.createSqlType(SqlTypeName.DECIMAL);
Review Comment:
Minor: `createSqlType(SqlTypeName.DECIMAL)` with no precision/scale resolves
to DECIMAL(2000, 1000) under Pinot's TypeSystem (getDefaultPrecision/Scale for
DECIMAL). It still maps to BIG_DECIMAL so results are correct (the BIG_DECIMAL
tests confirm it), but `needsCast` then compares the operand's real
precision/scale against (2000,1000) and will insert a redundant `CAST(x AS
DECIMAL(2000,1000))` even when the operand is already BIG_DECIMAL. Passing an
explicit precision/scale (or short-circuiting an already-BIG_DECIMAL operand)
would avoid the extra cast and the surprising scale-1000 intermediate type. Nit.
--
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]
