xiangfu0 commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4290464909
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -132,12 +133,49 @@ 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 numbers by value, like a comparison does and like the
single-stage engine does for IN. `equals()` also
+ /// compares the scale of a BigDecimal, so 1.0 (e.g. a SUM, which is a
DOUBLE) does not equal 1, and the class of a
+ /// number, so a LONG does not equal the same INT. The planner gives the
values of an IN the same type, so the
+ /// classes differ only if that changes.
+ private static boolean valuesEqual(Object value1, Object value2) {
+ if (value1 instanceof Number && value2 instanceof Number
+ && (value1 instanceof BigDecimal || value1.getClass() !=
value2.getClass())) {
+ return numbersEqual((Number) value1, (Number) value2);
+ }
+ return value1.equals(value2);
+ }
+
+ private static boolean numbersEqual(Number value1, Number value2) {
+ if (value1 instanceof BigDecimal || value2 instanceof BigDecimal) {
+ BigDecimal bigDecimal1 = toBigDecimal(value1);
+ BigDecimal bigDecimal2 = toBigDecimal(value2);
+ return bigDecimal1 != null && bigDecimal2 != null &&
bigDecimal1.compareTo(bigDecimal2) == 0;
+ }
+ if (value1 instanceof Double || value1 instanceof Float || value2
instanceof Double || value2 instanceof Float) {
+ return Double.compare(value1.doubleValue(), value2.doubleValue()) == 0;
Review Comment:
Nit/FYI: when one side is a Double/Float and the other is a Long above 2^53,
this collapses both to `double` and can report equality for values that differ
by a few units. That matches how Pinot's comparison operators already behave
(and your PR notes it's out of scope), so this is fine to leave as is — just
flagging it as a subtle edge for future readers.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -456,6 +465,105 @@ 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);
Review Comment:
This rewrites the operand in place (`call.setOperand`) during type checking.
If `checkOperandTypes` ever runs more than once on the same call (Calcite can
re-validate in some rewrite paths), you'd wrap the operand in a second CAST.
The `needsCast` guard likely prevents a double-wrap since operand 0 is already
the target type after the first pass, but it would be worth a short comment
confirming that re-entry is safe.
--
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]
