[CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… #3917
Conversation
…nding mode when cast an approximate numeric to int
|
| !ok | ||
|
|
||
|
|
||
| select cast(5.5 as smallint), |
There was a problem hiding this comment.
for each statement, add comments explaining what rounding mode is being used
| @@ -384,9 384,15 @@ static void checkRoundedRange(Number value, double min, double max) { | |||
| } | |||
| } | |||
|
|
|||
| /** Called from BuiltInMethod.INTEGER_CAST */ | |||
There was a problem hiding this comment.
javadoc is same as next method
if methods have the same description, are both required?
| * | ||
| * <p> {@link org.apache.calcite.sql.SqlDialect.DatabaseProduct#CALCITE}'s | ||
| * default is {@link RoundingMode#DOWN} and {@link org.apache.calcite.sql.SqlDialect.DatabaseProduct#POSTGRESQL}, | ||
| * {@link org.apache.calcite.sql.SqlDialect.DatabaseProduct#MYSQL} and {@link org.apache.calcite.sql.SqlDialect.DatabaseProduct#ORACLE} is {@link RoundingMode#HALF_UP}. |
There was a problem hiding this comment.
line is too long and can be wrapped
| } | ||
| return Expressions.constant(null, type); | ||
| } | ||
|
|
||
| static Expression getDefaultValue(Type type) { |
There was a problem hiding this comment.
should the method without roundingMode be deprecated?
|
|
||
| public RexExecutorImpl(DataContext dataContext) { | ||
| this.dataContext = dataContext; | ||
| this.sqlConformance = SqlConformanceEnum.DEFAULT; |
There was a problem hiding this comment.
I don't think conformance belongs here. Conformance governs validation behavior, not runtime behavior.
| @@ -133,7 152,7 @@ public static RexExecutable getExecutable(RexBuilder rexBuilder, List<RexNode> e | |||
| try { | |||
| code = compile(rexBuilder, constExps, (list, index, storageType) -> { | |||
| throw new UnsupportedOperationException(); | |||
| }); | |||
| }, sqlConformance); | |||
There was a problem hiding this comment.
code is more readable if the lambda is the last parameter
| @@ -505,9 505,9 @@ private Expression getConvertExpression( | |||
| case SMALLINT: { | |||
| if (SqlTypeName.NUMERIC_TYPES.contains(sourceType.getSqlTypeName())) { | |||
| return Expressions.call( | |||
| BuiltInMethod.INTEGER_CAST.method, | |||
| BuiltInMethod.INTEGER_CAST_ROUNDING_MODE.method, | |||
There was a problem hiding this comment.
this should apply to casts from decimal to decimal above
| @@ -144,4 146,8 @@ public abstract class SqlAbstractConformance implements SqlConformance { | |||
| @Override public boolean allowLenientCoercion() { | |||
| return SqlConformanceEnum.DEFAULT.allowLenientCoercion(); | |||
| } | |||
|
|
|||
| @Override public RoundingMode roundingMode() { | |||
| return SqlConformanceEnum.DEFAULT.roundingMode(); | |||
There was a problem hiding this comment.
I think rounding should be part of the type system
There was a problem hiding this comment.
I agree.
I commented elsewhere that including SqlConformance in RexExecutor made me feel uneasy. SqlConformance is about what is allowed in valid SQL.
There was a problem hiding this comment.
@julianhyde @mihaibudiu I will move the roundingMode to RelDataTypeSystem. Thanks for the advice.
| @@ -315,6 316,8 @@ public enum BuiltInMethod { | |||
| FP_DECIMAL_CAST(Primitive.class, "fpDecimalCast", | |||
| Number.class, int.class, int.class), | |||
| INTEGER_CAST(Primitive.class, "integerCast", Primitive.class, Object.class), | |||
| INTEGER_CAST_ROUNDING_MODE(Primitive.class, "integerCast", | |||
There was a problem hiding this comment.
The other cast method should probably be removed.
but many other cast methods have to be considered, including DECIMAL to DECIMAL, FP to integer, FP to decimal, DECIMAL to FP and perhaps even string to decimal
There was a problem hiding this comment.
Yes. According to your advice, I found some places that use the default RoudingMode.DOWN. I'll see what I can do.
…nding mode when cast an approximate numeric to int