yashmayya commented on code in PR #19790:
URL: https://github.com/apache/pinot/pull/19790#discussion_r4300012488
##########
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:
Thanks, agreed. Fixed in e5a1fe5b24: the planner now turns BIG_DECIMAL `IN`
and `NOT IN` lists into `=` and `<>` comparisons (`OR` and `AND`), like it
already does for BIG_DECIMAL range sets. Comparisons use `compareTo` on every
server version, so the result no longer depends on the scale during a rolling
upgrade.
To check it, I ran `NumericTypes.json` with the new planner and the old
`FilterOperand` from master (which uses `equals`): the `IN`, `HAVING ... IN`
and `NOT IN` cases pass. New `RexExpressionUtilsTest` cases check the converted
expressions.
##########
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:
Thanks, confirmed: this nested cast returns 1.2. The outer cast sees a
scale-0 operand type, so it treats the value as an integer and casts it
directly. Here the value still has a fraction because the inner cast to
`DECIMAL(p, 0)` with p > 38 keeps it, as it already does on master
(`CAST(doubleCol AS DECIMAL(40, 0))` alone returns 1.2).
The clean fix is to make that inner cast drop the fraction too. Servers only
get the target type name today, so that needs a new server-side function that
rounds a BigDecimal to a scale. I'd prefer to do it in a follow-up rather than
special-case nested casts here.
##########
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:
Thanks. This needs a multi-value column cast to a DECIMAL array together
with an explicit ARRAY_AGG data type, which is a rare combination, so I'm
leaving it out of this PR to keep it focused.
##########
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:
Thanks, agreed. It's the same as the comparison operators, so I'm leaving it
as is.
##########
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:
Good point. Re-entry is safe: on a second pass, operand 0 already has the
data type, so `needsCast()` returns false and no second CAST is added. I added
a comment saying this in e5a1fe5b24.
--
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]
