You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "NobiGo (via GitHub)" <gi...@apache.org> on 2024/08/16 12:38:54 UTC
[PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
NobiGo opened a new pull request, #3917:
URL: https://github.com/apache/calcite/pull/3917
…nding mode when cast an approximate numeric to int
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1720851432
##########
core/src/main/java/org/apache/calcite/util/BuiltInMethod.java:
##########
@@ -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",
Review Comment:
Yes. According to your advice, I found some places that use the default `RoudingMode.DOWN`. I'll see what I can do.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1723570108
##########
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:
I really don't see why it has to be in this class for quidem, in fact, the way you setup the type system I think you could move it to the CoreQuidemTest class.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1720089643
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -477,28 +483,41 @@ static BigDecimal checkOverflow(BigDecimal value, int precision, int scale) {
return checkOverflow(decimal, precision, scale);
}
+ public @Nullable Object numberValue(Number value) {
+ return numberValue(value, RoundingMode.DOWN);
+ }
/**
Review Comment:
need space between methods
##########
linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java:
##########
@@ -384,9 +384,15 @@ static void checkRoundedRange(Number value, double min, double max) {
}
}
+ /** Called from BuiltInMethod.INTEGER_CAST */
Review Comment:
javadoc is same as next method
if methods have the same description, are both required?
##########
core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java:
##########
@@ -56,11 +56,19 @@
public class RexExecutorImpl implements RexExecutor {
private final DataContext dataContext;
+ private final SqlConformance sqlConformance;
public RexExecutorImpl(DataContext dataContext) {
this.dataContext = dataContext;
+ this.sqlConformance = SqlConformanceEnum.DEFAULT;
Review Comment:
I don't think conformance belongs here. Conformance governs validation behavior, not runtime behavior.
##########
core/src/test/java/org/apache/calcite/test/CoreQuidemTest.java:
##########
@@ -88,6 +88,33 @@ public static void main(String[] args) throws Exception {
SqlConformanceEnum.MYSQL_5)
.with(CalciteAssert.Config.SCOTT)
.connect();
+ case "scott-default":
+ // Same as "scott", but uses Calcite's default conformance.
+ return CalciteAssert.that()
+ .with(CalciteConnectionProperty.PARSER_FACTORY,
+ ExtensionDdlExecutor.class.getName() + "#PARSER_FACTORY")
+ .with(CalciteConnectionProperty.CONFORMANCE,
+ SqlConformanceEnum.DEFAULT)
+ .with(CalciteAssert.Config.SCOTT)
+ .connect();
+ case "scott-sqlserver2008":
+ // Same as "scott", but uses Calcite's default conformance.
Review Comment:
comment is copy-pasted and identical to other comments
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java:
##########
@@ -583,4 +585,20 @@ public interface SqlConformance {
*/
@Experimental
boolean allowLenientCoercion();
+
+ /**
+ * Specifies a rounding behavior for numerical operations capable of discarding precision.
+ *
+ * <p>Among the built-in conformance levels,
+ * {@link RoundingMode#HALF_UP} in
+ * {@link SqlConformanceEnum#MYSQL_5},
+ * {@link SqlConformanceEnum#ORACLE_10},
+ * {@link SqlConformanceEnum#ORACLE_12};
+ * {@link RoundingMode#DOWN} otherwise.
+ *
+ * <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}.
Review Comment:
line is too long and can be wrapped
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2658,4 +2658,151 @@ FROM "hr"."emps";
!ok
+# [CALCITE-4838] Add RoundingMode in SqlConformance to document the rounding mode when cast an approximate numeric to int
+!use scott-mysql
+
+select cast(5.5 as int),
+ cast(3.5 as int),
+ cast(1.6 as int),
+ cast(1.1 as int),
+ cast(1.0 as int),
+ cast(-1.0 as int),
+ cast(-1.1 as int),
+ cast(-1.6 as int),
+ cast(-2.5 as int),
+ cast(15.5 as int)
+from (values 0) as t(x);
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+| EXPR$0 | EXPR$1 | EXPR$2 | EXPR$3 | EXPR$4 | EXPR$5 | EXPR$6 | EXPR$7 | EXPR$8 | EXPR$9 |
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+| 6 | 4 | 2 | 1 | 1 | -1 | -1 | -2 | -3 | 16 |
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+(1 row)
+
+!ok
+
+select cast(5.5 as tinyint),
+ cast(3.5 as tinyint),
+ cast(1.6 as tinyint),
+ cast(1.1 as tinyint),
+ cast(1.0 as tinyint),
+ cast(-1.0 as tinyint),
+ cast(-1.1 as tinyint),
+ cast(-1.6 as tinyint),
+ cast(-2.5 as tinyint),
+ cast(15.5 as tinyint)
+from (values 0) as t(x);
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+| EXPR$0 | EXPR$1 | EXPR$2 | EXPR$3 | EXPR$4 | EXPR$5 | EXPR$6 | EXPR$7 | EXPR$8 | EXPR$9 |
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+| 6 | 4 | 2 | 1 | 1 | -1 | -1 | -2 | -3 | 16 |
++--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
+(1 row)
+
+!ok
+
+
+select cast(5.5 as smallint),
Review Comment:
for each statement, add comments explaining what rounding mode is being used
##########
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) {
+ Primitive p = Primitive.of(type);
+ if (p != null) {
+ return Expressions.constant(p.defaultValue, type, roundingMode);
+ }
+ return Expressions.constant(null, type);
+ }
+
static Expression getDefaultValue(Type type) {
Review Comment:
should the method without `roundingMode` be deprecated?
##########
core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java:
##########
@@ -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);
Review Comment:
code is more readable if the lambda is the last parameter
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722476941
##########
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:
I will move it to a testing class.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722720090
##########
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) {
+ Primitive p = Primitive.of(type);
+ if (p != null) {
+ return Expressions.constant(p.defaultValue, type, roundingMode);
+ }
+ return Expressions.constant(null, type);
+ }
+
static Expression getDefaultValue(Type type) {
Review Comment:
Revert code.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2298167227
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1724150211
##########
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:
Done.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2300047954
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722467868
##########
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:
Ok. I will add a new one.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo merged PR #3917:
URL: https://github.com/apache/calcite/pull/3917
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1720326428
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java:
##########
@@ -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();
Review Comment:
I agree.
I commented elsewhere that including `SqlConformance` in `RexExecutor` made me feel uneasy. `SqlConformance` is about what is allowed in valid SQL.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2298055391
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2295848973
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2295514195
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2295554486
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722429538
##########
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:
Yes, Calclite‘s default rounding behavior is DOWN. I added this for the unit test.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722467719
##########
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:
Yes, that's why I expected the DEFAULT to be DOWN.
But if DEFAULT was used only for testing and it will move to a testing class, this is fine.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1720852521
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java:
##########
@@ -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();
Review Comment:
@julianhyde @mihaibudiu I will move the `roundingMode` to `RelDataTypeSystem`. Thanks for the advice.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1720126405
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -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,
Review Comment:
this should apply to casts from decimal to decimal above
##########
core/src/main/java/org/apache/calcite/util/BuiltInMethod.java:
##########
@@ -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",
Review 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
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlAbstractConformance.java:
##########
@@ -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();
Review Comment:
I think rounding should be part of the type system
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2293488754
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [9 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722464357
##########
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:
Do you mean using the default RoundingMode? There are already APIs in Calcite that use DOWN, so I set the default rounding is DOWN. `Primitive#numberValue` uses the DOWN mode to convert the decimal、float and double to long. WDYT？ And I check the `BigDecimal#intValue` uses the DOWN to convert number to int. So the default RoundingMode is DOWN.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2298073599
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2300024154
> There may be other places where rounding happens silently, but I guess we won't discover these easily. (For example I think BigDecimal.longValue does rounding silently). One way to discover them would be to write a lot of tests that cast every type possible to every type possible that requires rounding, using multiple values around the corner cases. But that could be left for a separate PR.
@mihaibudiu Yes. I know. Many places still use default APIs, which are not completely consistent with this configuration. We are currently unable to assess the boundaries of this issue. Once we discover the problem, we will submit a new PR to fix it.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2300025263
I have tagged this with "merge soon", let's see if anyone else has comments.
Perhaps you can squash the commits too, since the PR isn't too large and no other people have commented yet.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722431192
##########
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:
then I think this should be in a test class
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2298408785
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722978904
##########
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:
This may be a suitable place because I want to test this PR through Quidem. To avoid any misunderstandings, I have added the relevant Java document 'Only for testing'` to make it clear.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in RelDataTypeSystem to specify the rounding behavior [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2300025351
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3917:
URL: https://github.com/apache/calcite/pull/3917#discussion_r1722425520
##########
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 will revert the changes and check the unit test.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4838] Add RoundingMode in SqlConformance to document the rou… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3917:
URL: https://github.com/apache/calcite/pull/3917#issuecomment-2293746338
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917) **Quality Gate passed**
Issues
 [7 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3917&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3917&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3917&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3917)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org