mihaibudiu commented on code in PR #3945:
URL: https://github.com/apache/calcite/pull/3945#discussion_r1768897597
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java:
##########
@@ -88,6 +90,17 @@ public SqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
if (maxPrecision >= 0 && precision > maxPrecision) {
precision = maxPrecision;
}
+ if (typeName == SqlTypeName.DECIMAL) {
+ if (precision != RelDataType.PRECISION_NOT_SPECIFIED && precision <= 0) {
+ throw RESOURCE.invalidPrecisionForDecimalType(precision,
maxPrecision).ex();
+ }
+ if (scale != RelDataType.SCALE_NOT_SPECIFIED && scale < 0) {
+ if (!typeSystem.supportsNegativeScale() || scale <
typeSystem.getMinNumericScale()) {
Review Comment:
is the first test necessary?
comparing the scale with the minimum one may be enough?
##########
core/src/test/java/org/apache/calcite/test/CustomRelDataTypeSystem.java:
##########
@@ -23,18 +23,42 @@
/**
* Custom type system only for Quidem test.
- *
- * <p> Specify the rounding behaviour. In the default implementation,
- * the rounding mode is {@link RoundingMode#DOWN}, but here is {@link
RoundingMode#HALF_UP}
- *
- * <p>The default implementation is {@link #DEFAULT}.
*/
-public class CustomRelDataTypeSystem extends RelDataTypeSystemImpl {
-
- public static final RelDataTypeSystem ROUNDING_MODE_HALF_UP = new
CustomRelDataTypeSystem();
+public final class CustomRelDataTypeSystem {
- @Override public RoundingMode roundingMode() {
- return RoundingMode.HALF_UP;
+ private CustomRelDataTypeSystem() {
}
+
+ /**
+ * Specify the rounding behaviour. In the default implementation,
+ * the rounding mode is {@link RoundingMode#DOWN}, but here is {@link
RoundingMode#HALF_UP}.
+ */
+ public static final RelDataTypeSystem ROUNDING_MODE_HALF_UP = new
RelDataTypeSystemImpl() {
+ @Override public RoundingMode roundingMode() {
+ return RoundingMode.HALF_UP;
+ }
+ };
+
+ /**
+ * Supports negative scale and rounding mode is {@link RoundingMode#DOWN}.
+ */
+ public static final RelDataTypeSystem NEGATIVE_SCALE = new
RelDataTypeSystemImpl() {
+ @Override public int getMinNumericScale() {
Review Comment:
this is a very large number... but I guess it works for testing
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1702,6 +1703,51 @@ void testCastToBoolean(CastType castType,
SqlOperatorFixture f) {
}
}
+ @Test void testCastToDecimal() {
+ final SqlOperatorFixture f = fixture();
+ // test the minimum scale is 0
+ f.setFor(SqlStdOperatorTable.CAST, VmName.EXPAND);
+ // cast integer to decimal
+ f.checkFails("cast(123 as decimal(3, -1))",
+ "DECIMAL scale -1 must be between 0 and 19", false);
+ // cast float to decimal
+ f.checkFails("cast(12.3 as decimal(3, -1))",
+ "DECIMAL scale -1 must be between 0 and 19", false);
+ // cast decimal to decimal
+ f.checkFails("cast(cast(12.3 as decimal(3, 1)) as decimal(3, -1))",
+ "DECIMAL scale -1 must be between 0 and 19", false);
+ // cast string to decimal
+ f.checkFails("cast('12.3' as decimal(3, -1))",
+ "DECIMAL scale -1 must be between 0 and 19", false);
+ // cast interval to decimal
+ f.checkFails("cast(INTERVAL '5' hour as decimal(3, -1))",
+ "DECIMAL scale -1 must be between 0 and 19", false);
+ // test the minimum scale is -2
+ final SqlOperatorFixture negativeScaleFixture = fixture()
+ .withFactory(tf ->
+ tf.withTypeSystem(typeSystem ->
+ new RelDataTypeSystemImpl() {
+ @Override public int getMinNumericScale() {
+ return -2;
+ }
+ }));
+ // cast integer to decimal
+ negativeScaleFixture.checkScalar("cast(123 as decimal(3, -1))",
+ "120", "DECIMAL(3, -1) NOT NULL");
+ // cast float to decimal
+ negativeScaleFixture.checkScalar("cast(12.3 as decimal(3, -1))",
+ "10", "DECIMAL(3, -1) NOT NULL");
+ // cast decimal to decimal
+ negativeScaleFixture.checkScalar("cast(cast(12.3 as decimal(3, 1)) as
decimal(3, -1))",
+ "10", "DECIMAL(3, -1) NOT NULL");
+ // cast string to decimal
+ negativeScaleFixture.checkScalar("cast('12.3' as decimal(3, -1))",
+ "10", "DECIMAL(3, -1) NOT NULL");
+ // cast interval to decimal
+ negativeScaleFixture.checkScalar("cast(INTERVAL '5' hour as decimal(3,
-1))",
Review Comment:
is 0 the right result?
##########
core/src/test/resources/sql/cast.iq:
##########
@@ -1234,4 +1234,385 @@ EnumerableCalc(expr#0..7=[{inputs}],
expr#8=[CAST($t6):INTEGER], expr#9=[IS NOT
EnumerableTableScan(table=[[scott, EMP]])
!plan
+# [CALCITE-6560] Add supportsNegativeScale in RelDataTypeSystem
+
+!use scott-negative-scale
+
+# supports negative scale and the rounding mode is DOWN
+
+# throw an exception when CAST target decimal type has illegal precision and
scale
+
+# precision is 0
+values cast(15.3 as decimal(0, 4));
+DECIMAL precision 0 must be between 1 and 19
+!error
+
+# Same as previous; the number is 0
+values cast(0 as decimal(0,0));
+DECIMAL precision 0 must be between 1 and 19
+!error
+
+# scale greater than precision
+values cast(0 as decimal(3, 4));
++--------+
+| EXPR$0 |
++--------+
+| 0.0000 |
++--------+
+(1 row)
+
+!ok
+
+# Same as previous; the number is 1
+select cast(1 as decimal(3, 4));
+Value 1 cannot be represented as a DECIMAL(3, 4)
+!error
+
+# test the maximum value that can be stored
+select cast(99999 as decimal(2, -3));
++--------+
+| EXPR$0 |
++--------+
+| 99000 |
++--------+
+(1 row)
+
+!ok
+
+# Same as previous; the value is minimum value -99999
+select cast(-99999 as decimal(2, -3));
++--------+
+| EXPR$0 |
++--------+
+| -99000 |
++--------+
+(1 row)
+
+!ok
+
+# Same as previous; the value is 100000
+select cast(100000 as decimal(2, -3));
+Value 100000 cannot be represented as a DECIMAL(2, -3)
+!error
+
+# Same as previous; the value is -100000
+select cast(-100000 as decimal(2, -3));
+Value -100000 cannot be represented as a DECIMAL(2, -3)
+!error
+
+# Test cast integer to decimal with negative scale
+
+# The non-rounded digits greater than the precision represents the maximum
value
+select cast(12340 as decimal(3, -1));
+Value 12340 cannot be represented as a DECIMAL(3, -1)
+!error
+
+# The precision represents the maximum value greater than the number
+select cast(12340 as decimal(6, -1));
++--------+
+| EXPR$0 |
++--------+
+| 12340 |
++--------+
+(1 row)
+
+!ok
+
+# Same as previous; the precision is 5
+select cast(12340 as decimal(5, -1));
++--------+
+| EXPR$0 |
++--------+
+| 12340 |
++--------+
+(1 row)
+
+!ok
+
+# Same as previous; the precision is 4
+select cast(12340 as decimal(4, -1));
++--------+
+| EXPR$0 |
++--------+
+| 12340 |
++--------+
+(1 row)
+
+!ok
+
+# Test the negative scale that can be represents
+select cast(12340 as decimal(3, -2));
++--------+
+| EXPR$0 |
++--------+
+| 12300 |
++--------+
+(1 row)
+
+!ok
+
Review Comment:
frankly, I am surprised these work so well.
There are certainly some operations that won't work with negative scales
(some in code I wrote), but these are not in the scope of this PR.
--
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]
