Address review comments

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
apache · May 10, 2024 · e0c65b9 · e0c65b9
1 parent 11125eb
commit e0c65b9
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 40 deletions.
diff --git a/core/src/main/java/org/apache/calcite/util/BuiltInMethod.java b/core/src/main/java/org/apache/calcite/util/BuiltInMethod.java
@@ -295,9 +295,9 @@ public enum BuiltInMethod {
   DECIMAL_DECIMAL_CAST(Primitive.class, "decimalDecimalCast",
       BigDecimal.class, int.class, int.class),
   INTEGER_DECIMAL_CAST(Primitive.class, "integerDecimalCast",
-      Object.class, int.class, int.class),
+      Number.class, int.class, int.class),
   FP_DECIMAL_CAST(Primitive.class, "fpDecimalCast",
-      Object.class, int.class, int.class),
+      Number.class, int.class, int.class),
   INTEGER_CAST(Primitive.class, "integerCast", Primitive.class, Object.class),
   MEMORY_GET0(MemoryFactory.Memory.class, "get"),
   MEMORY_GET1(MemoryFactory.Memory.class, "get", int.class),

diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java
@@ -390,13 +390,17 @@ static void checkRoundedRange(Number value, double min, double max) {
   }
 
   static BigDecimal checkOverflow(BigDecimal value, int precision, int scale) {
-    BigDecimal result = value.setScale(scale, RoundingMode.FLOOR);
+    // The rounding mode is not specified in any Calcite docs,
+    // but elsewhere Calcite is rounding down.  For example, Calcite is frequently
+    // calling BigDecimal.longValue(), which is rounding down, by ignoring all
+    // digits after the decimal point.
+    BigDecimal result = value.setScale(scale, RoundingMode.DOWN);
     result = result.stripTrailingZeros();
     if (result.scale() < scale) {
       // stripTrailingZeros also removes zeros if there is no
       // decimal point, converting 1000 to 1e+3, using a negative scale.
       // Here we undo this change.
-      result = result.setScale(scale, RoundingMode.FLOOR);
+      result = result.setScale(scale, RoundingMode.DOWN);
     }
     int actualPrecision = result.precision();
     if (actualPrecision > precision) {
@@ -417,21 +421,21 @@ static BigDecimal checkOverflow(BigDecimal value, int precision, int scale) {
 
   /** Called from BuiltInMethod.INTEGER_DECIMAL_CAST */
   public static @Nullable Object integerDecimalCast(
-      @Nullable Object value, int precision, int scale) {
+      @Nullable Number value, int precision, int scale) {
     if (value == null) {
       return null;
     }
-    final BigDecimal decimal = new BigDecimal(((Number) value).longValue());
+    final BigDecimal decimal = new BigDecimal(value.longValue());
     return checkOverflow(decimal, precision, scale);
   }
 
   /** Called from BuiltInMethod.FP_DECIMAL_CAST */
   public static @Nullable Object fpDecimalCast(
-      @Nullable Object value, int precision, int scale) {
+      @Nullable Number value, int precision, int scale) {
     if (value == null) {
       return null;
     }
-    final BigDecimal decimal = BigDecimal.valueOf(((Number) value).doubleValue());
+    final BigDecimal decimal = BigDecimal.valueOf(value.doubleValue());
     return checkOverflow(decimal, precision, scale);
   }
 

diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
@@ -598,13 +598,6 @@ void testCastBooleanToNumeric(CastType castType, SqlOperatorFixture f) {
         "Cast function cannot convert value of type BOOLEAN to type DECIMAL\\(19, 0\\)", false);
   }
 
-  /** Test case for <a href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6322">
-   *  [CALCITE-6322] Casts to DECIMAL types are ignored</a>. */
-  @Test void testDecimalDecimalCast() {
-    final SqlOperatorFixture f = fixture();
-    f.checkScalar("CAST(1.123 AS DECIMAL(4, 0))", "1", "DECIMAL(4, 0) NOT NULL");
-  }
-
   @ParameterizedTest
   @MethodSource("safeParameters")
   void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
@@ -711,6 +704,29 @@ void testCastToExactNumeric(CastType castType, SqlOperatorFixture f) {
         "654342432412312");
   }
 
+  /** Test cases for <a href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6322">
+   * [CALCITE-6322] Casts to DECIMAL types are ignored</a>. */
+  @Test public void testIssue6322() {
+    SqlOperatorFixture f = fixture();
+    f.checkScalar("CAST(1.123 AS DECIMAL(4, 0))", "1", "DECIMAL(4, 0) NOT NULL");
+    f.checkScalar("CAST(100 AS DECIMAL(3, 0))", "100", "DECIMAL(3, 0) NOT NULL");
+    f.checkScalar("CAST(-100 AS DECIMAL(3, 0))", "-100", "DECIMAL(3, 0) NOT NULL");
+    f.checkScalar("CAST(100 AS DECIMAL(5, 2))", "100", "DECIMAL(5, 2) NOT NULL");
+    f.checkScalar("CAST(-100 AS DECIMAL(5, 2))", "-100", "DECIMAL(5, 2) NOT NULL");
+    f.checkFails("CAST(1000 AS DECIMAL(2, 0))",
+        "Value 1000 cannot be represented as a DECIMAL\\(2, 0\\)", true);
+    f.checkFails("CAST(-1000 AS DECIMAL(2, 0))",
+        "Value -1000 cannot be represented as a DECIMAL\\(2, 0\\)", true);
+    f.checkScalar("CAST(100.5e0 AS DECIMAL(4, 1))", "100.5", "DECIMAL(4, 1) NOT NULL");
+    f.checkScalar("CAST(-100.5e0 AS DECIMAL(4, 1))", "-100.5", "DECIMAL(4, 1) NOT NULL");
+    f.checkScalar("CAST(100.55e0 AS DECIMAL(4, 1))", "100.5", "DECIMAL(4, 1) NOT NULL");
+    f.checkScalar("CAST(-100.55e0 AS DECIMAL(4, 1))", "-100.5", "DECIMAL(4, 1) NOT NULL");
+    f.checkFails("CAST(100.5e0 AS DECIMAL(4, 2))",
+        "Value 100.5 cannot be represented as a DECIMAL\\(4, 2\\)", true);
+    f.checkFails("CAST(-100.5e0 AS DECIMAL(4, 2))",
+        "Value -100.5 cannot be represented as a DECIMAL\\(4, 2\\)", true);
+  }
+
   /**
    * Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-5843">
    * Constant expression with nested casts causes a compiler crash</a>. */
@@ -769,7 +785,6 @@ void testCastStringToDecimal(CastType castType, SqlOperatorFixture f) {
         "-1.2");
     f.checkFails("cast(' -1.21e' as decimal(2,1))", INVALID_CHAR_MESSAGE,
         true);
-    f.checkFails("cast(''100' as decimal(2, 1))", "Value .* out of range", true);
   }
 
   @ParameterizedTest
@@ -1128,9 +1143,7 @@ void testCastNull(CastType castType, SqlOperatorFixture f) {
 
     // null
     f.checkNull("cast(null as integer)");
-    if (DECIMAL) {
-      f.checkNull("cast(null as decimal(4,3))");
-    }
+    f.checkNull("cast(null as decimal(4,3))");
     f.checkNull("cast(null as double)");
     f.checkNull("cast(null as varchar(10))");
     f.checkNull("cast(null as char(10))");
@@ -1733,13 +1746,13 @@ void testCastToBoolean(CastType castType, SqlOperatorFixture f) {
     f.checkString("case 1 when 1 then cast('a' as varchar(1)) "
             + "when 2 then cast('bcd' as varchar(3)) end",
         "a", "VARCHAR(3)");
+    f.checkScalarExact("case 2 when 1 then 11.2 "
+            + "when 2 then 4.543 else null end",
+        "DECIMAL(5, 3)", "4.543");
     if (DECIMAL) {
-      f.checkScalarExact("case 2 when 1 then 11.2 "
-              + "when 2 then 4.543 else null end",
-          "DECIMAL(5, 3)", "4.543");
       f.checkScalarExact("case 1 when 1 then 11.2 "
               + "when 2 then 4.543 else null end",
-          "DECIMAL(5, 3)", "11.200");
+          "DECIMAL(5, 3)", "11.2");
     }
     f.checkScalarExact("case 'a' when 'a' then 1 end", 1);
     f.checkScalarApprox("case 1 when 1 then 11.2e0 "
@@ -2498,9 +2511,6 @@ void checkModOperator(SqlOperatorFixture f) {
     f.checkScalarExact("12%-7", 5);
     f.checkScalarExact("cast(12 as tinyint) % cast(-7 as tinyint)",
         "TINYINT NOT NULL", "5");
-    if (!DECIMAL) {
-      return;
-    }
     f.checkScalarExact("cast(9 as decimal(2, 0)) % 7",
         "INTEGER NOT NULL", "2");
     f.checkScalarExact("7 % cast(9 as decimal(2, 0))",
@@ -2517,9 +2527,6 @@ void checkModPrecedence(SqlOperatorFixture f) {
   void checkModOperatorNull(SqlOperatorFixture f) {
     f.checkNull("cast(null as integer) % 2");
     f.checkNull("4 % cast(null as tinyint)");
-    if (!DECIMAL) {
-      return;
-    }
     f.checkNull("4 % cast(null as decimal(12,0))");
   }
 
@@ -2555,7 +2562,6 @@ void checkModOperatorDivByZero(SqlOperatorFixture f) {
           "10010000000.00000000");
     }
     f.checkNull("1e1 / cast(null as float)");
-
     f.checkScalarExact("100.1 / 0.00000000000000001", "DECIMAL(19, 0) NOT NULL",
         "1.001E+19");
   }
@@ -3021,9 +3027,6 @@ static void checkOverlaps(OverlapChecker c) {
   }
 
   @Test void testLessThanOperatorInterval() {
-    if (!DECIMAL) {
-      return;
-    }
     final SqlOperatorFixture f = fixture();
     f.checkBoolean("interval '2' day < interval '1' day", false);
     f.checkBoolean("interval '2' day < interval '5' day", true);
@@ -6565,9 +6568,6 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
     f.checkScalarExact("mod(cast(12 as tinyint), cast(-7 as tinyint))",
         "TINYINT NOT NULL", "5");
 
-    if (!DECIMAL) {
-      return;
-    }
     f.checkScalarExact("mod(cast(9 as decimal(2, 0)), 7)",
         "INTEGER NOT NULL", "2");
     f.checkScalarExact("mod(7, cast(9 as decimal(2, 0)))",
@@ -6581,16 +6581,13 @@ void checkRegexpExtract(SqlOperatorFixture f0, FunctionAlias functionAlias) {
     final SqlOperatorFixture f = fixture();
     f.checkNull("mod(cast(null as integer),2)");
     f.checkNull("mod(4,cast(null as tinyint))");
-    if (!DECIMAL) {
-      return;
-    }
     f.checkNull("mod(4,cast(null as decimal(12,0)))");
   }
 
   @Test void testModFuncDivByZero() {
     // The extra CASE expression is to fool Janino.  It does constant
     // reduction and will throw the divide by zero exception while
-    // compiling the expression.  The test frame work would then issue
+    // compiling the expression.  The test framework would then issue
     // unexpected exception occurred during "validation".  You cannot
     // submit as non-runtime because the janino exception does not have
     // error position information and the framework is unhappy with that.