You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/04/18 21:08:11 UTC
[PR] [CALCITE-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
mihaibudiu opened a new pull request, #3764:
URL: https://github.com/apache/calcite/pull/3764
This is a piece of https://github.com/apache/calcite/pull/3589 which only contains the fix to CALCITE-6169
This also fixes CALCITE-6366
The main part of the fix is implementing Expressions.convertChecked.
There was a recent implementation of this method, but we replace it in the generated Java code with a call to Primitive.numberValue, which covers more cases than the existing implementation.
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3764:
URL: https://github.com/apache/calcite/pull/3764#issuecomment-2240120984
@NobiGo I hope I have addressed your comments
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3764:
URL: https://github.com/apache/calcite/pull/3764#issuecomment-2229522516
It would be nice to get a review on this PR, this is part of a long series of fixes to the implementation of type conversions in Calcite.
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3764:
URL: https://github.com/apache/calcite/pull/3764#issuecomment-2229610870
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3764) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3764&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3764&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3764&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3764&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3764&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3764)
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3764:
URL: https://github.com/apache/calcite/pull/3764#discussion_r1685874505
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java:
##########
@@ -481,10 +482,23 @@ public static Expression convert(Expression operand, Type fromType,
if (toPrimitive != null) {
if (fromPrimitive != null) {
// E.g. from "float" to "double"
- return Expressions.convert_(
+ if (toPrimitive == Primitive.BOOLEAN) {
Review Comment:
I will check this.
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3764:
URL: https://github.com/apache/calcite/pull/3764#issuecomment-2243540950
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3764) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3764&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3764&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3764&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3764&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3764&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3764)
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3764:
URL: https://github.com/apache/calcite/pull/3764
--
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-6169] EnumUtils.convert does not implement the correct SQL cast semantics [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3764:
URL: https://github.com/apache/calcite/pull/3764#discussion_r1678830535
##########
core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java:
##########
@@ -99,15 +99,22 @@ public boolean isExact() {
if (isExact) {
int scaleValue = requireNonNull(scale, "scale");
if (0 == scaleValue) {
- BigDecimal bd = getValueNonNull();
- SqlTypeName result;
- long l = bd.longValue();
- if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE)) {
- result = SqlTypeName.INTEGER;
- } else {
- result = SqlTypeName.BIGINT;
+ try {
+ BigDecimal bd = getValueNonNull();
+ SqlTypeName result;
+ // Will throw if the number cannot
+ // be represented as a long.
+ long l = bd.longValue();
+ if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE)) {
+ result = SqlTypeName.INTEGER;
+ } else {
+ result = SqlTypeName.BIGINT;
+ }
+ return typeFactory.createSqlType(result);
+ } catch (ArithmeticException ex) {
+ // This indicates that the value does
+ // not fit in any integer type. Fallback to DECIMAL.
Review Comment:
Same as above we don't need break line and before `Fallback` there are extra spaces.
##########
core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java:
##########
@@ -99,15 +99,22 @@ public boolean isExact() {
if (isExact) {
int scaleValue = requireNonNull(scale, "scale");
if (0 == scaleValue) {
- BigDecimal bd = getValueNonNull();
- SqlTypeName result;
- long l = bd.longValue();
- if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE)) {
- result = SqlTypeName.INTEGER;
- } else {
- result = SqlTypeName.BIGINT;
+ try {
+ BigDecimal bd = getValueNonNull();
+ SqlTypeName result;
+ // Will throw if the number cannot
Review Comment:
Maybe here does not need a break.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java:
##########
@@ -481,10 +482,23 @@ public static Expression convert(Expression operand, Type fromType,
if (toPrimitive != null) {
if (fromPrimitive != null) {
// E.g. from "float" to "double"
- return Expressions.convert_(
+ if (toPrimitive == Primitive.BOOLEAN) {
+ return Expressions.convert_(operand, toPrimitive.getPrimitiveClass());
+ }
+ return Expressions.convertChecked(
operand, toPrimitive.getPrimitiveClass());
}
- if (fromNumber || fromBox == Primitive.CHAR) {
+ if (fromType == BigDecimal.class && toPrimitive.isFixedNumeric()) {
+ // Conversion from decimal to an exact type
+ ConstantExpression zero = Expressions.constant(0);
+ // Elsewhere Calcite uses this rounding mode implicitly, so we have to be consistent.
+ // E.g., this is the rounding mode used by BigDecimal.longValue().
+ Expression rounding = Expressions.constant(RoundingMode.DOWN);
+ // rounded = operand.setScale(0, RoundingMode.DOWN);
Review Comment:
Keep consistent with other comment, Maybe should start with
```
// Generate
```
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java:
##########
@@ -481,10 +482,23 @@ public static Expression convert(Expression operand, Type fromType,
if (toPrimitive != null) {
if (fromPrimitive != null) {
// E.g. from "float" to "double"
- return Expressions.convert_(
+ if (toPrimitive == Primitive.BOOLEAN) {
Review Comment:
Maybe need to add code comment about Why boolean and other type is different?
--
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