yashmayya commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4287225260
##########
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:
Good point, done. `valuesEqual` now compares two numbers by value when their
classes differ or when one of them is a BigDecimal:
- as BigDecimal values when one side is a BigDecimal (NaN and infinity never
match),
- as doubles when one side is a FLOAT or DOUBLE,
- as longs otherwise.
Two values of the same class (other than BigDecimal) still use `equals`, so
the common path does not change. New `FilterOperatorTest` cases cover a
BigDecimal with another scale, a BigDecimal against a LONG, and `NOT IN` with
INT and DOUBLE values against a LONG.
##########
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:
Good catch, done. For `'BIG_DECIMAL'`, the checker now casts a DECIMAL value
only when the planner gives it INT or LONG (scale 0 and up to 18 digits). The
other DECIMAL types plan as arrays (`DOUBLE_ARRAY`) that the BIG_DECIMAL result
converts to, so they keep their type and get no cast.
When a cast is necessary, I kept DECIMAL(2000, 1000). It is Pinot's
BIG_DECIMAL type (the type of a BIG_DECIMAL column, and of
`ColumnDataType.BIG_DECIMAL.toType()`), so a BIG_DECIMAL column gets no cast.
The runtime CAST only gets the name `BIG_DECIMAL`, so the scale does not change
any value.
A new plan test shows no cast for a BIG_DECIMAL column and for a DECIMAL(20,
0) value, and a cast for a DECIMAL(5, 0) value. A runtime test checks the
values.
--
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]
