xiangfu0 commented on code in PR #19791:
URL: https://github.com/apache/pinot/pull/19791#discussion_r4278272154
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java:
##########
@@ -44,14 +48,73 @@ public static boolean isDistinctFrom(@Nullable Object obj1,
@Nullable Object obj
if (obj1 == null || obj2 == null) {
return true;
}
- return !obj1.equals(obj2);
+ if (isNumber(obj1) && isNumber(obj2)) {
+ return !numberEquals((Number) obj1, (Number) obj2);
+ }
+ return !Objects.deepEquals(obj1, obj2);
}
@ScalarFunction(nullableParameters = true)
public static boolean isNotDistinctFrom(@Nullable Object obj1, @Nullable
Object obj2) {
return !isDistinctFrom(obj1, obj2);
}
+ private static boolean isNumber(Object obj) {
+ return obj instanceof Integer || obj instanceof Long || obj instanceof
Float || obj instanceof Double
+ || obj instanceof BigDecimal;
+ }
+
+ private static boolean isFloatingPoint(Number number) {
+ return number instanceof Float || number instanceof Double;
+ }
+
+ /// Compares two numbers by value, so that numbers of different types (e.g.
`Integer` and `Long`) can be equal. This
+ /// is close to `BinaryOperatorTransformFunction`, which runs
`isDistinctFrom` in the single-stage engine and in the
+ /// multi-stage leaf stage. It differs only in two edge cases: a `long`
above 2^53 is compared exactly with a
+ /// floating point number, and NaN or infinity compared with a `BigDecimal`
does not throw.
+ /// - Two integers are compared as `long`.
+ /// - Two floating point numbers are compared with [Double#compare], like
[Double#equals]: a `float` is widened
+ /// (`1.1f` does not equal `1.1`), NaN equals NaN, and `-0.0` does not
equal `0.0`.
+ /// - An integer and a floating point number are compared exactly.
+ /// - When either number is a `BigDecimal`, both are compared as
`BigDecimal`, which ignores the scale (`1.5`
+ /// equals `1.50`). A floating point number is converted with
[BigDecimal#valueOf(double)], so `0.1` equals the
+ /// `BigDecimal` `0.1`. NaN and infinity do not equal any `BigDecimal`.
+ private static boolean numberEquals(Number number1, Number number2) {
+ if (number1 instanceof BigDecimal || number2 instanceof BigDecimal) {
+ BigDecimal value1 = toBigDecimal(number1);
+ BigDecimal value2 = toBigDecimal(number2);
Review Comment:
Non-blocking: worth being aware that this BigDecimal path makes `-0.0`
behave inconsistently depending on the other operand's type.
`isDistinctFrom(-0.0, 0.0)` is `true` (the double path uses `Double.compare`),
but `isDistinctFrom(-0.0, CAST(0 AS DECIMAL))` is `false` here, since
`BigDecimal.valueOf(-0.0)` normalizes to `0`. Your test at `{-0.0,
BigDecimal.ZERO, false}` already pins this. It matches what the leaf stage does
via `BigDecimal.valueOf(double)`, so I'd keep it as-is for consistency — just
flagging it as a subtle corner that might surprise a future reader.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java:
##########
@@ -44,14 +48,73 @@ public static boolean isDistinctFrom(@Nullable Object obj1,
@Nullable Object obj
if (obj1 == null || obj2 == null) {
return true;
}
- return !obj1.equals(obj2);
+ if (isNumber(obj1) && isNumber(obj2)) {
+ return !numberEquals((Number) obj1, (Number) obj2);
+ }
+ return !Objects.deepEquals(obj1, obj2);
}
@ScalarFunction(nullableParameters = true)
public static boolean isNotDistinctFrom(@Nullable Object obj1, @Nullable
Object obj2) {
return !isDistinctFrom(obj1, obj2);
}
+ private static boolean isNumber(Object obj) {
+ return obj instanceof Integer || obj instanceof Long || obj instanceof
Float || obj instanceof Double
+ || obj instanceof BigDecimal;
Review Comment:
Minor: `isNumber` intentionally only covers the boxed types that actually
flow through the scalar layer (INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL), so
`Byte`/`Short`/`BigInteger` fall through to the `equals`/`deepEquals` path.
That's correct for Pinot's stored types today — just noting it in case someone
later wires up a new numeric type and expects by-value comparison here
automatically.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java:
##########
@@ -44,14 +48,73 @@ public static boolean isDistinctFrom(@Nullable Object obj1,
@Nullable Object obj
if (obj1 == null || obj2 == null) {
return true;
}
- return !obj1.equals(obj2);
+ if (isNumber(obj1) && isNumber(obj2)) {
+ return !numberEquals((Number) obj1, (Number) obj2);
+ }
+ return !Objects.deepEquals(obj1, obj2);
}
@ScalarFunction(nullableParameters = true)
public static boolean isNotDistinctFrom(@Nullable Object obj1, @Nullable
Object obj2) {
return !isDistinctFrom(obj1, obj2);
}
+ private static boolean isNumber(Object obj) {
+ return obj instanceof Integer || obj instanceof Long || obj instanceof
Float || obj instanceof Double
+ || obj instanceof BigDecimal;
+ }
+
+ private static boolean isFloatingPoint(Number number) {
+ return number instanceof Float || number instanceof Double;
+ }
+
+ /// Compares two numbers by value, so that numbers of different types (e.g.
`Integer` and `Long`) can be equal. This
+ /// is close to `BinaryOperatorTransformFunction`, which runs
`isDistinctFrom` in the single-stage engine and in the
+ /// multi-stage leaf stage. It differs only in two edge cases: a `long`
above 2^53 is compared exactly with a
+ /// floating point number, and NaN or infinity compared with a `BigDecimal`
does not throw.
+ /// - Two integers are compared as `long`.
+ /// - Two floating point numbers are compared with [Double#compare], like
[Double#equals]: a `float` is widened
+ /// (`1.1f` does not equal `1.1`), NaN equals NaN, and `-0.0` does not
equal `0.0`.
+ /// - An integer and a floating point number are compared exactly.
+ /// - When either number is a `BigDecimal`, both are compared as
`BigDecimal`, which ignores the scale (`1.5`
+ /// equals `1.50`). A floating point number is converted with
[BigDecimal#valueOf(double)], so `0.1` equals the
+ /// `BigDecimal` `0.1`. NaN and infinity do not equal any `BigDecimal`.
+ private static boolean numberEquals(Number number1, Number number2) {
+ if (number1 instanceof BigDecimal || number2 instanceof BigDecimal) {
+ BigDecimal value1 = toBigDecimal(number1);
+ BigDecimal value2 = toBigDecimal(number2);
Review Comment:
Non-blocking: worth being aware that this BigDecimal path makes `-0.0`
behave inconsistently depending on the other operand's type.
`isDistinctFrom(-0.0, 0.0)` is `true` (the double path uses `Double.compare`),
but `isDistinctFrom(-0.0, CAST(0 AS DECIMAL))` is `false` here, since
`BigDecimal.valueOf(-0.0)` normalizes to `0`. Your test at `{-0.0,
BigDecimal.ZERO, false}` already pins this. It matches what the leaf stage does
via `BigDecimal.valueOf(double)`, so I'd keep it as-is for consistency — just
flagging it as a subtle corner that might surprise a future reader.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java:
##########
@@ -44,14 +48,73 @@ public static boolean isDistinctFrom(@Nullable Object obj1,
@Nullable Object obj
if (obj1 == null || obj2 == null) {
return true;
}
- return !obj1.equals(obj2);
+ if (isNumber(obj1) && isNumber(obj2)) {
+ return !numberEquals((Number) obj1, (Number) obj2);
+ }
+ return !Objects.deepEquals(obj1, obj2);
}
@ScalarFunction(nullableParameters = true)
public static boolean isNotDistinctFrom(@Nullable Object obj1, @Nullable
Object obj2) {
return !isDistinctFrom(obj1, obj2);
}
+ private static boolean isNumber(Object obj) {
+ return obj instanceof Integer || obj instanceof Long || obj instanceof
Float || obj instanceof Double
+ || obj instanceof BigDecimal;
Review Comment:
Minor: `isNumber` intentionally only covers the boxed types that actually
flow through the scalar layer (INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL), so
`Byte`/`Short`/`BigInteger` fall through to the `equals`/`deepEquals` path.
That's correct for Pinot's stored types today — just noting it in case someone
later wires up a new numeric type and expects by-value comparison here
automatically.
--
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]
