Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-2067] RexLiteral cannot represent accurately floating point values, including NaN, Infinity #3663

Merged
merged 1 commit into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,8 @@ private static String getLiteralType(Object literal) {
}
} else if (String.class.equals(literal.getClass())) {
return "string";
} else if (literal instanceof Double) {
return "float";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why float?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same string that is used 3 lines above

}
throw new UnsupportedOperationException("Unsupported literal " + literal);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ static void initializeArrowState(@TempDir Path sharedTempDir)
String sql = "select * from arrowdata\n"
+ " where \"floatField\"=15.0";
String plan = "PLAN=ArrowToEnumerableConverter\n"
+ " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0)])\n"
+ " ArrowFilter(condition=[=(CAST($2):DOUBLE, 15.0E0)])\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of E0 added. Is it a goal of this change to make decimal literals print differently from double literals?

Maybe I missed it, but somewhere in the javadoc (e.g. for RexLiteral) should state that goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't changed how floating point values are serialized as text. The difference comes because now more literals contain a double/float.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

+1 when you fix the issues in my other 2 comments

+ " ArrowTableScan(table=[[ARROW, ARROWDATA]], fields=[[0, 1, 2, 3]])\n\n";
String result = "intField=15; stringField=15; floatField=15.0; longField=15\n";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1498,7 +1498,7 @@ public static SqlNode toSql(RexLiteral literal) {
case EXACT_NUMERIC: {
if (SqlTypeName.APPROX_TYPES.contains(typeName)) {
return SqlLiteral.createApproxNumeric(
castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
castNonNull(literal.getValueAs(Double.class)).toString(), POS);
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
} else {
return SqlLiteral.createExactNumeric(
castNonNull(literal.getValueAs(BigDecimal.class)).toPlainString(), POS);
Expand Down
38 changes: 33 additions & 5 deletions core/src/main/java/org/apache/calcite/rex/RexBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,21 @@ public RexLiteral makeApproxLiteral(BigDecimal bd) {
public RexLiteral makeApproxLiteral(@Nullable BigDecimal bd, RelDataType type) {
assert SqlTypeFamily.APPROXIMATE_NUMERIC.getTypeNames().contains(
type.getSqlTypeName());
return makeLiteral(bd, type, SqlTypeName.DOUBLE);
return makeLiteral(bd != null ? bd.doubleValue() : null, type, SqlTypeName.DOUBLE);
}

/**
* Creates an approximate numeric literal (double or float)
* from a Double value.
*
* @param val literal value
* @param type approximate numeric type
* @return new literal
*/
public RexLiteral makeApproxLiteral(Double val, RelDataType type) {
assert SqlTypeFamily.APPROXIMATE_NUMERIC.getTypeNames().contains(
type.getSqlTypeName());
return makeLiteral(val, type, SqlTypeName.DOUBLE);
}

/**
Expand Down Expand Up @@ -2013,7 +2027,10 @@ public RexNode makeLiteral(@Nullable Object value, RelDataType type,
case FLOAT:
case REAL:
case DOUBLE:
return makeApproxLiteral((BigDecimal) value, type);
if (value instanceof Double) {
return makeApproxLiteral((Double) value, type);
}
return makeApproxLiteral(((BigDecimal) value).doubleValue(), type);
case BOOLEAN:
return (Boolean) value ? booleanTrue : booleanFalse;
case TIME:
Expand Down Expand Up @@ -2159,13 +2176,24 @@ public RexNode makeLambdaCall(RexNode expr, List<RexLambdaRef> parameters) {
type.getSqlTypeName());
return new BigDecimal(((Number) o).longValue());
case REAL:
case FLOAT:
case DOUBLE:
if (o instanceof BigDecimal) {
return o;
}
return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL64)
// Float values are stored as Doubles
if (o instanceof Float) {
return ((Float) o).doubleValue();
}
if (o instanceof Double) {
return o;
}
return new BigDecimal(((Number) o).doubleValue(), MathContext.DECIMAL32)
.stripTrailingZeros();
case FLOAT:
case DOUBLE:
if (o instanceof Double) {
return o;
}
return ((Number) o).doubleValue();
case CHAR:
case VARCHAR:
if (o instanceof NlsString) {
Expand Down
79 changes: 56 additions & 23 deletions core/src/main/java/org/apache/calcite/rex/RexLiteral.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@

import java.io.PrintWriter;
import java.math.BigDecimal;
import java.math.MathContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
import java.text.SimpleDateFormat;
Expand Down Expand Up @@ -112,9 +113,11 @@
* <td>{@link BigDecimal}</td>
* </tr>
* <tr>
* <td>{@link SqlTypeName#DOUBLE}</td>
* <td>{@link SqlTypeName#DOUBLE},
* {@link SqlTypeName#REAL},
* {@link SqlTypeName#FLOAT}</td>
* <td>Approximate number, for example <code>6.023E-23</code>.</td>
* <td>{@link BigDecimal}</td>
* <td>{@link Double}.</td>
* </tr>
* <tr>
* <td>{@link SqlTypeName#DATE}</td>
Expand Down Expand Up @@ -193,7 +196,7 @@ public class RexLiteral extends RexNode {
/**
* The value of this literal. Must be consistent with its type, as per
* {@link #valueMatchesType}. For example, you can't store an
* {@link Integer} value here just because you feel like it -- all numbers are
* {@link Integer} value here just because you feel like it -- all exact numbers are
* represented by a {@link BigDecimal}. But since this field is private, it
* doesn't really matter how the values are stored.
*/
Expand All @@ -204,12 +207,9 @@ public class RexLiteral extends RexNode {
*/
private final RelDataType type;

// TODO jvs 26-May-2006: Use SqlTypeFamily instead; it exists
// for exactly this purpose (to avoid the confusion which results
// from overloading SqlTypeName).
/**
* An indication of the broad type of this literal -- even if its type isn't
* a SQL type. Sometimes this will be different than the SQL type; for
* a SQL type. Sometimes this will be different from the SQL type; for
* example, all exact numbers, including integers have typeName
* {@link SqlTypeName#DECIMAL}. See {@link #valueMatchesType} for the
* definitive story.
Expand Down Expand Up @@ -294,10 +294,10 @@ public final String computeDigest(
}

/**
* Returns true if {@link RexDigestIncludeType#OPTIONAL} digest would include data type.
* Returns whether {@link RexDigestIncludeType} digest would include data type.
*
* @see RexCall#computeDigest(boolean)
* @return true if {@link RexDigestIncludeType#OPTIONAL} digest would include data type
* @return whether {@link RexDigestIncludeType} digest would include data type
*/
@RequiresNonNull("type")
RexDigestIncludeType digestIncludesType(
Expand Down Expand Up @@ -328,11 +328,12 @@ public static boolean valueMatchesType(
}
// fall through
case DECIMAL:
case BIGINT:
return value instanceof BigDecimal;
case DOUBLE:
case FLOAT:
case REAL:
case BIGINT:
return value instanceof BigDecimal;
return value instanceof Double;
case DATE:
return value instanceof DateString;
case TIME:
Expand Down Expand Up @@ -660,8 +661,14 @@ private static void appendAsJava(@Nullable Comparable value, StringBuilder sb,
break;
case DOUBLE:
case FLOAT:
assert value instanceof BigDecimal;
sb.append(Util.toScientificNotation((BigDecimal) value));
if (value instanceof BigDecimal) {
sb.append(Util.toScientificNotation((BigDecimal) value));
} else {
assert value instanceof Double;
Double d = (Double) value;
String repr = Util.toScientificNotation(d);
sb.append(repr);
}
break;
case BIGINT:
assert value instanceof BigDecimal;
Expand Down Expand Up @@ -1075,22 +1082,48 @@ public boolean isNull() {
case BIGINT:
case INTEGER:
case SMALLINT:
case TINYINT:
case DOUBLE:
case REAL:
case FLOAT:
case TINYINT: {
BigDecimal bd = (BigDecimal) value;
if (clazz == Long.class) {
return clazz.cast(((BigDecimal) value).longValue());
return clazz.cast(bd.longValue());
} else if (clazz == Integer.class) {
return clazz.cast(((BigDecimal) value).intValue());
return clazz.cast(bd.intValue());
} else if (clazz == Short.class) {
return clazz.cast(((BigDecimal) value).shortValue());
return clazz.cast(bd.shortValue());
} else if (clazz == Byte.class) {
return clazz.cast(((BigDecimal) value).byteValue());
return clazz.cast(bd.byteValue());
} else if (clazz == Double.class) {
return clazz.cast(((BigDecimal) value).doubleValue());
return clazz.cast(bd.doubleValue());
} else if (clazz == Float.class) {
return clazz.cast(((BigDecimal) value).floatValue());
return clazz.cast(bd.floatValue());
}
break;
}
case DOUBLE:
case REAL:
case FLOAT:
if (value instanceof Double) {
Double d = (Double) value;
if (clazz == Long.class) {
return clazz.cast(d.longValue());
} else if (clazz == Integer.class) {
return clazz.cast(d.intValue());
} else if (clazz == Short.class) {
return clazz.cast(d.shortValue());
} else if (clazz == Byte.class) {
return clazz.cast(d.byteValue());
} else if (clazz == Double.class) {
// Cast still needed, since the Java compiler does not understand
// that T is double.
return clazz.cast(d);
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
} else if (clazz == Float.class) {
return clazz.cast(d.floatValue());
} else if (clazz == BigDecimal.class) {
// This particular conversion is lossy, since in general BigDecimal cannot
// represent accurately FP values. However, this is the best we can do.
// This conversion used to be in RexBuilder, used when creating a RexLiteral.
return clazz.cast(new BigDecimal(d, MathContext.DECIMAL64).stripTrailingZeros());
}
}
break;
case DATE:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ public RexLiteral literal(@Nullable Object value) {
return rexBuilder.makeExactLiteral((BigDecimal) value);
} else if (value instanceof Float || value instanceof Double) {
return rexBuilder.makeApproxLiteral(
BigDecimal.valueOf(((Number) value).doubleValue()));
((Number) value).doubleValue(), getTypeFactory().createSqlType(SqlTypeName.DOUBLE));
} else if (value instanceof Number) {
return rexBuilder.makeExactLiteral(
BigDecimal.valueOf(((Number) value).longValue()));
Expand Down
19 changes: 19 additions & 0 deletions core/src/main/java/org/apache/calcite/util/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,21 @@ public static void println(
pw.println();
}

/**
* Formats a double value to a String ensuring that the output
* is in scientific notation if the value is not "special".
* (Special values include infinities and NaN.)
*/
public static String toScientificNotation(Double d) {
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
String repr = Double.toString(d);
if (!repr.toLowerCase(Locale.ENGLISH).contains("e")
&& !d.isInfinite()
&& !d.isNaN()) {
repr += "E0";
}
return repr;
}

/**
* Formats a {@link BigDecimal} value to a string in scientific notation For
* example<br>
Expand All @@ -558,6 +573,10 @@ public static String toScientificNotation(BigDecimal bd) {
int len = unscaled.length();
int scale = bd.scale();
int e = len - scale - 1;
if (bd.stripTrailingZeros().equals(BigDecimal.ZERO)) {
// Without this adjustment 0.0 generates 0E-1
e = 0;
}

StringBuilder ret = new StringBuilder();
if (bd.signum() < 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ private static String toSql(RelNode root, SqlDialect dialect,
+ "where \"net_weight\" <> 10 or \"net_weight\" is null";
final String expected = "SELECT \"product_id\", \"shelf_width\"\n"
+ "FROM \"foodmart\".\"product\"\n"
+ "WHERE \"net_weight\" <> 10 OR \"net_weight\" IS NULL";
+ "WHERE \"net_weight\" <> CAST(10 AS DOUBLE) OR \"net_weight\" IS NULL";
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
sql(query).ok(expected);
}

Expand Down Expand Up @@ -4391,7 +4391,7 @@ private SqlDialect nonOrdinalDialect() {
+ " select \"product_id\", 0 as \"net_weight\"\n"
+ " from \"sales_fact_1997\") t0";
final String expected = "SELECT SUM(CASE WHEN \"product_id\" = 0"
+ " THEN \"net_weight\" ELSE 0 END) AS \"NET_WEIGHT\"\n"
+ " THEN \"net_weight\" ELSE 0E0 END) AS \"NET_WEIGHT\"\n"
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
+ "FROM (SELECT \"product_id\", \"net_weight\"\n"
+ "FROM \"foodmart\".\"product\"\n"
+ "UNION ALL\n"
Expand Down Expand Up @@ -6507,7 +6507,7 @@ private void checkLiteral2(String expression, String expected) {
+ "PATTERN (\"STRT\" \"DOWN\" + \"UP\" +)\n"
+ "DEFINE "
+ "\"DOWN\" AS PREV(\"DOWN\".\"net_weight\", 0) = "
+ "0 OR PREV(\"DOWN\".\"net_weight\", 0) = 1, "
+ "CAST(0 AS DOUBLE) OR PREV(\"DOWN\".\"net_weight\", 0) = CAST(1 AS DOUBLE), "
+ "\"UP\" AS PREV(\"UP\".\"net_weight\", 0) > "
+ "PREV(\"UP\".\"net_weight\", 1))";
sql(sql).ok(expected);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,11 @@ interface Action {
final RexCall first =
(RexCall) rexBuilder.makeCall(SqlStdOperatorTable.LN,
rexBuilder.makeLiteral(3, integer, true));
// Division by zero causes an exception during evaluation
final RexCall second =
(RexCall) rexBuilder.makeCall(SqlStdOperatorTable.LN,
rexBuilder.makeLiteral(-2, integer, true));
(RexCall) rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE_INTEGER,
rexBuilder.makeLiteral(-2, integer, true),
rexBuilder.makeLiteral(0, integer, true));
executor.reduce(rexBuilder, ImmutableList.of(first, second),
reducedValues);
assertThat(reducedValues, hasSize(2));
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/apache/calcite/test/JdbcTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ static void forEachExpand(Runnable r) {
+ "expr#7=[null:JavaType(class java.lang.Integer)], "
+ "empid=[$t3], deptno=[$t4], name=[$t5], salary=[$t6], "
+ "commission=[$t7])\n"
+ " EnumerableValues(tuples=[[{ 'Fred', 56, 123.4 }]])\n";
+ " EnumerableValues(tuples=[[{ 'Fred', 56, 123.4000015258789E0 }]])\n";
assertThat(resultSet.getString(1), isLinux(expected));

// With named columns
Expand Down
27 changes: 27 additions & 0 deletions core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3341,6 +3341,33 @@ private void checkPushJoinThroughUnionOnRightDoesNotMatchSemiOrAntiJoin(JoinRelT
.check();
}

/** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-2067">
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
* [CALCITE-2067] RexLiteral cannot represent accurately floating point values,
* including NaN, Infinity</a>. */
@Test public void testDoubleReduction() {
// Without the fix for CALCITE-2067 the result returned below is
// 1008618.49. Ironically, that result is more accurate; however
// it is not the result returned by the pow() function, which is
// 1008618.4899999999
final String sql = "SELECT power(1004.3, 2)";
sql(sql)
.withRule(CoreRules.PROJECT_REDUCE_EXPRESSIONS)
.check();
}

/** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-2067">
* [CALCITE-2067] RexLiteral cannot represent accurately floating point values,
* including NaN, Infinity</a>. */
@Test public void testDoubleReduction2() {
// Without the fix for CALCITE-2067 the following expression is not
// reduced to Infinity, since Infinity cannot be represented
// as a BigDecimal value.
final String sql2 = "SELECT 1.0 / 0.0e0";
sql(sql2)
.withRule(CoreRules.PROJECT_REDUCE_EXPRESSIONS)
.check();
}

/** Tests that {@link UnionMergeRule} does nothing if its arguments have
* are different set operators, {@link Union} and {@link Intersect}. */
@Test void testMergeSetOpMixed() {
Expand Down
Loading
Loading