Copilot commented on code in PR #7935:
URL: https://github.com/apache/ignite-3/pull/7935#discussion_r3043421660
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteExpressions.java:
##########
@@ -161,6 +179,42 @@ private static Expression negateExact(ExpressionType
unaryType, Expression opera
return Expressions.makeUnary(unaryType, operand);
}
+ /**
+ * Generates a comparison expression for floating-point types using {@link
Float#compare(float, float)}
+ * or {@link Double#compare(double, double)} instead of primitive
comparison operators.
+ *
+ * <p>This is necessary because Java primitive comparisons ({@code ==},
{@code <}, etc.) do not handle
+ * {@code NaN} values in a way consistent with SQL semantics. For example,
{@code Float.NaN == Float.NaN}
+ * is {@code false} in Java primitive arithmetic, but SQL expects {@code
NaN = NaN} to be {@code true}.
+ *
+ * <p>Using {@code Float.compare}/{@code Double.compare} treats {@code
NaN} as equal to itself and greater
+ * than all other values, which matches the expected SQL behavior.
+ *
+ * @param binaryType The comparison expression type.
+ * @param left Left operand.
+ * @param right Right operand.
+ * @return A comparison expression using {@code compare()}, or {@code
null} if the operands are not floating-point.
+ */
+ private static @Nullable Expression compareFloatingPoint(ExpressionType
binaryType, Expression left, Expression right) {
+ Type leftType = left.getType();
+ Type rightType = right.getType();
+
+ boolean isFloat = leftType == Float.TYPE || rightType == Float.TYPE;
+ boolean isDouble = leftType == Double.TYPE || rightType == Double.TYPE;
+
+ if (!isFloat && !isDouble) {
+ return null;
+ }
+
+ // Double takes precedence over Float
+ Class<?> compareClass = isDouble ? Double.class : Float.class;
+
+ // Generate: Float.compare(left, right) op 0 or Double.compare(left,
right) op 0
+ Expression cmp = Expressions.call(compareClass, "compare", left,
right);
+
Review Comment:
compareFloatingPoint currently (1) only detects primitive float/double types
(missing Float.class/Double.class), and (2) passes operands to
Float/Double.compare without ensuring they match the selected method signature.
This can break expression generation when one side is boxed (Float/Double) or
when comparing a floating column to a non-floating numeric literal (e.g., float
vs int), since Expressions.call may not resolve a compatible compare(...)
overload. Fix by recognizing both primitive + boxed floating types and
explicitly converting/casting both operands to the target primitive type
(double if either is double; else float) before calling compare(...).
```suggestion
boolean isFloat = leftType == Float.TYPE || leftType == Float.class
|| rightType == Float.TYPE || rightType == Float.class;
boolean isDouble = leftType == Double.TYPE || leftType ==
Double.class
|| rightType == Double.TYPE || rightType == Double.class;
if (!isFloat && !isDouble) {
return null;
}
// Double takes precedence over Float.
Class<?> targetType = isDouble ? Double.TYPE : Float.TYPE;
Class<?> compareClass = isDouble ? Double.class : Float.class;
Expression convertedLeft = Expressions.convert_(left, targetType);
Expression convertedRight = Expressions.convert_(right, targetType);
// Generate: Float.compare((float) left, (float) right) op 0
// or: Double.compare((double) left, (double) right) op 0
Expression cmp = Expressions.call(compareClass, "compare",
convertedLeft, convertedRight);
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -420,7 +420,15 @@ private Object toJson(RexNode node) {
RexLiteral literal = (RexLiteral) node;
Object value = literal.getValue3();
map = map();
- map.put("literal", toJson(value));
+ // For approximate numeric types, serialize special
floating-point values (NaN, ±Infinity, -0.0)
+ // as strings to preserve them through JSON round-trip.
Jackson deserializes floating-point numbers
+ // as BigDecimal (due to USE_BIG_DECIMAL_FOR_FLOATS), which
cannot represent these special values.
+ if (value instanceof Double &&
isApproximateNumeric(node.getType())
+ && isSpecialFloatingPointValue((Double) value)) {
+ map.put("literal", value.toString());
+ } else {
+ map.put("literal", toJson(value));
+ }
Review Comment:
This special-value serialization path only handles Double literals. For
REAL/FLOAT literals (typically represented as Float), NaN/±Infinity/-0.0f will
still be serialized as a JSON number and can be deserialized into BigDecimal,
reintroducing the exact round-trip issue described in the comment. Extend the
logic to handle Float values too (and detect -0.0f via Float.floatToRawIntBits).
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteExpressions.java:
##########
@@ -161,6 +179,42 @@ private static Expression negateExact(ExpressionType
unaryType, Expression opera
return Expressions.makeUnary(unaryType, operand);
}
+ /**
+ * Generates a comparison expression for floating-point types using {@link
Float#compare(float, float)}
+ * or {@link Double#compare(double, double)} instead of primitive
comparison operators.
+ *
+ * <p>This is necessary because Java primitive comparisons ({@code ==},
{@code <}, etc.) do not handle
+ * {@code NaN} values in a way consistent with SQL semantics. For example,
{@code Float.NaN == Float.NaN}
+ * is {@code false} in Java primitive arithmetic, but SQL expects {@code
NaN = NaN} to be {@code true}.
+ *
+ * <p>Using {@code Float.compare}/{@code Double.compare} treats {@code
NaN} as equal to itself and greater
+ * than all other values, which matches the expected SQL behavior.
+ *
+ * @param binaryType The comparison expression type.
+ * @param left Left operand.
+ * @param right Right operand.
+ * @return A comparison expression using {@code compare()}, or {@code
null} if the operands are not floating-point.
+ */
Review Comment:
This doc explains the NaN motivation but omits another user-visible change
this implementation introduces: Float/Double.compare treat -0.0 and +0.0 as
different (compare(...) != 0), affecting '=' and '<>' semantics. Please
document the signed-zero behavior explicitly here (and/or in the related issue
context) so future changes don’t accidentally revert it or misinterpret the
intent.
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/SqlExpressionFactoryImplTest.java:
##########
@@ -828,6 +829,132 @@ private static Stream<Arguments> numericLiterals() {
);
}
+ @ParameterizedTest
+ @MethodSource("doubleSpecialValuesArgs")
+ public void testDoubleSpecialValuesComparison(
+ SqlOperator op,
+ double leftVal,
+ double rightVal,
+ boolean expectedResult
+ ) {
+ RexBuilder rexBuilder = Commons.rexBuilder();
+ IgniteTypeFactory tf = Commons.typeFactory();
+
+ RelDataType colType = tf.createSqlType(SqlTypeName.DOUBLE);
+ RelDataType rowType = new Builder(tf)
+ .add("c1", colType)
+ .add("c2", colType)
+ .build();
+
+ RexInputRef ref1 = rexBuilder.makeInputRef(rowType, 0);
+ RexInputRef ref2 = rexBuilder.makeInputRef(rowType, 1);
+ RexNode filter = rexBuilder.makeCall(op, List.of(ref1, ref2));
+
+ SqlPredicate predicate = expFactory.predicate(filter, rowType);
+ assertEquals(expectedResult, predicate.test(ctx, new Object[]{leftVal,
rightVal}),
+ "Failed for " + leftVal + " " + op.getName() + " " + rightVal);
+ }
+
+ private static List<Arguments> doubleSpecialValuesArgs() {
+ return makeSpecialFloatArgs(
+ Double.NaN,
+ Double.POSITIVE_INFINITY,
+ Double.NEGATIVE_INFINITY,
+ 1.0d,
+ 0.0d
Review Comment:
The new comparison semantics also change equality/ordering around signed
zero (-0.0 vs +0.0), but these parameterized 'special values' cases only
include +0.0. Consider adding -0.0 (and the corresponding expected outcomes for
'='/'<>'/ordering) to these argument sets so the expression-factory path is
directly covered for signed-zero behavior.
```suggestion
0.0d,
-0.0d
```
--
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]
