mihaibudiu commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722279133
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -1435,6 +1435,14 @@ public Expression handle(Expression x) {
}
}
+ static Expression getDefaultValue(Type type, RoundingMode roundingMode) {
Review Comment:
I find it surprising that the default value depends on the rounding mode.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystemImpl.java:
##########
@@ -160,6 +162,10 @@ public abstract class RelDataTypeSystemImpl implements
RelDataTypeSystem {
return 19;
}
+ @Override public RoundingMode roundingMode() {
Review Comment:
I would expect this to return the default.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -385,22 +385,20 @@ static void checkRoundedRange(Number value, double min,
double max) {
}
/** Called from BuiltInMethod.INTEGER_CAST */
- public static @Nullable Object integerCast(Primitive primitive, final Object
value) {
- return requireNonNull(primitive, "primitive").numberValue((Number) value);
+ public static @Nullable Object integerCast(Primitive primitive, final Object
value,
Review Comment:
an alternative is to leave the old API around and just add the new one.
I prefer this solution, but it may cause problems for other projects that
try to upgrade to a new version of Calcite.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -4023,8 +4032,9 @@ private ParameterExpression genValueStatement(
return value;
}
- Expression getIfTrue(Type type, final List<Expression> argValueList) {
- return getDefaultValue(type);
+ Expression getIfTrue(Type type, final List<Expression> argValueList,
Review Comment:
if the default value does not depend on the rounding mode, then probably
this function does not either.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java:
##########
@@ -35,6 +37,13 @@ public interface RelDataTypeSystem {
/** Default type system. */
RelDataTypeSystem DEFAULT = new RelDataTypeSystemImpl() { };
+ /** Same as {@link #DEFAULT}. But the rounding mode is {@link
RoundingMode#HALF_UP}. */
+ RelDataTypeSystem DEFAULT_ROUNDING_MODE_HALF_UP = new
RelDataTypeSystemImpl() {
Review Comment:
until recently the default rounding mode has been DOWN, at least for most
conversions I have reviewed and implemented.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -412,69 +410,75 @@ static BigDecimal checkOverflow(BigDecimal value, int
precision, int scale) {
/** Called from BuiltInMethod.CHAR_DECIMAL_CAST */
public static @Nullable Object charToDecimalCast(
- @Nullable String value, int precision, int scale) {
+ @Nullable String value, int precision, int scale, RoundingMode
roundingMode) {
if (value == null) {
return null;
}
BigDecimal result = new BigDecimal(value.trim());
- return checkOverflow(result, precision, scale);
+ return checkOverflow(result, precision, scale, roundingMode);
}
/**
* Convert a short time interval to a decimal value.
* Called from BuiltInMethod.SHORT_INTERVAL_DECIMAL_CAST.
* @param unitScale Scale describing source interval type */
public static @Nullable Object shortIntervalToDecimalCast(
- @Nullable Long value, int precision, int scale, BigDecimal unitScale) {
+ @Nullable Long value, int precision, int scale,
+ BigDecimal unitScale, RoundingMode roundingMode) {
if (value == null) {
return null;
}
// Divide with the scale expected of the result
- BigDecimal result = new BigDecimal(value).divide(unitScale, scale,
RoundingMode.DOWN);
- return checkOverflow(result, precision, scale);
+ BigDecimal result = new BigDecimal(value).divide(unitScale, scale,
roundingMode);
+ return checkOverflow(result, precision, scale, roundingMode);
}
/**
* Convert a long time interval to a decimal value.
* Called from BuiltInMethod.LONG_INTERVAL_DECIMAL_CAST.
* @param unitScale Scale describing source interval type */
public static @Nullable Object longIntervalToDecimalCast(
- @Nullable Integer value, int precision, int scale, BigDecimal unitScale)
{
+ @Nullable Integer value, int precision, int scale,
+ BigDecimal unitScale, RoundingMode roundingMode) {
if (value == null) {
return null;
}
// Divide with the scale expected of the result
- BigDecimal result = new BigDecimal(value).divide(unitScale, scale,
RoundingMode.DOWN);
- return checkOverflow(result, precision, scale);
+ BigDecimal result = new BigDecimal(value).divide(unitScale, scale,
roundingMode);
+ return checkOverflow(result, precision, scale, roundingMode);
}
/** Called from BuiltInMethod.DECIMAL_DECIMAL_CAST */
public static @Nullable Object decimalDecimalCast(
- @Nullable BigDecimal value, int precision, int scale) {
+ @Nullable BigDecimal value, int precision, int scale, RoundingMode
roundingMode) {
if (value == null) {
return null;
}
- return checkOverflow(value, precision, scale);
+ return checkOverflow(value, precision, scale, roundingMode);
}
/** Called from BuiltInMethod.INTEGER_DECIMAL_CAST */
public static @Nullable Object integerDecimalCast(
- @Nullable Number value, int precision, int scale) {
+ @Nullable Number value, int precision, int scale, RoundingMode
roundingMode) {
if (value == null) {
return null;
}
final BigDecimal decimal = new BigDecimal(value.longValue());
- return checkOverflow(decimal, precision, scale);
+ return checkOverflow(decimal, precision, scale, roundingMode);
}
/** Called from BuiltInMethod.FP_DECIMAL_CAST */
public static @Nullable Object fpDecimalCast(
- @Nullable Number value, int precision, int scale) {
+ @Nullable Number value, int precision, int scale, RoundingMode
roundingMode) {
if (value == null) {
return null;
}
final BigDecimal decimal = BigDecimal.valueOf(value.doubleValue());
- return checkOverflow(decimal, precision, scale);
+ return checkOverflow(decimal, precision, scale, roundingMode);
+ }
+
+ public @Nullable Object numberValue(Number value) {
Review Comment:
This looks like a dangerous API.
If you insist on having this function, I would call it numberValueRoundDown
to make it clear what it does.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -385,22 +385,20 @@ static void checkRoundedRange(Number value, double min,
double max) {
}
/** Called from BuiltInMethod.INTEGER_CAST */
- public static @Nullable Object integerCast(Primitive primitive, final Object
value) {
- return requireNonNull(primitive, "primitive").numberValue((Number) value);
+ public static @Nullable Object integerCast(Primitive primitive, final Object
value,
Review Comment:
here you are changing some public APIs, you probably need to document this
in the history.md file as a breaking change
--
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]
