You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/03/17 18:15:28 UTC
[GitHub] [calcite] anilbash opened a new pull request #1859: [CALCITE-3863] Truncate/Round return type inference through rel data …
anilbash opened a new pull request #1859: [CALCITE-3863] Truncate/Round return type inference through rel data …
URL: https://github.com/apache/calcite/pull/1859
…type factory
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397182094
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,103 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Let p1, s1 be the precision and scale of the first operand</li>
+ * <li>Let s2 be the scale value to truncate</li>
+ * <li>Let p, s be the precision and scale of the result</li>
+ * <li>Let d be the number of whole digits in the result</li>
+ * <li>Then the result type is a decimal with:
Review comment:
removed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 edited a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 edited a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603748315
> @danny0405 This is similar to #1311 which made some of the other decimal related rules configurable using a custom type system.
>
> Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same.
In my opinion, #1311 is very different because `+` and `-` are builtin operators and we have no way for downstream projects to replace these operators, but `Round` and `Truncate` are different, they are sql functions, and if your project has maintained the sql operators by yourself, there is a chance you just override the type inference of these two instead of the whole type system, which seems too heavy and special to customize.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603077610
I'm confused by your change, if you just want the `Round` or `Truncate` return type inference can be configurable, just replace the `Return type inference`, which is already pluggable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r396948372
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,103 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Let p1, s1 be the precision and scale of the first operand</li>
+ * <li>Let s2 be the scale value to truncate</li>
+ * <li>Let p, s be the precision and scale of the result</li>
+ * <li>Let d be the number of whole digits in the result</li>
+ * <li>Then the result type is a decimal with:
+ * <ul>
+ * <li>s = s2</li>
+ * <li>p = p1</li>
+ * </ul>
+ * </li>
+ * <li>p and s are capped at their maximum values</li>
+ * </ul>
+ *
+ * @param typeFactory TypeFactory used to create output type
+ * @param type1 Type of the first operand
+ * @param scale2 Scale value to truncate to
+ * @return Result type for a decimal truncate
+ */
+ default RelDataType deriveDecimalTruncateType(RelDataTypeFactory typeFactory,
+ RelDataType type1, Integer scale2) {
+ if (SqlTypeUtil.isExactNumeric(type1)) {
+ if (SqlTypeUtil.isDecimal(type1)) {
+ // Java numeric will always have invalid precision/scale,
Review comment:
you just return a decimal with same precision and scale of `type1`, so why we need these methods in `RelDataTypeSystem`, just return the `type1` seems okey ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash removed a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash removed a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603264439
> I'm confused by your change, if you just want the `Round` or `Truncate` return type inference can be configurable, just replace the `Return type inference`, which is already pluggable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r394286883
##########
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##########
@@ -557,6 +557,32 @@ public int size() {
public static final SqlReturnTypeInference NULLABLE_MOD =
chain(DECIMAL_MOD_NULLABLE, ARG1_NULLABLE);
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE = opBinding -> {
+ RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+ RelDataType type1 = opBinding.getOperandType(0);
+ Integer scale = 0;
+ if (opBinding.getOperandCount() > 1) {
+ scale = opBinding.getOperandLiteralValue(1, Integer.class);
+ }
+ return typeFactory.getTypeSystem().deriveDecimalTruncateType(typeFactory, type1, scale);
+ };
+
+ /**
+ * Type-inference strategy whereby the result type of a call is the decimal
+ * truncate of two exact numeric operands where at least one of the operands is a
+ * decimal.
+ */
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE_NULLABLE =
+ cascade(DECIMAL_TRUNCATE, SqlTypeTransforms.TO_NULLABLE);
+
+ /**
+ * Type-inference strategy whereby the result type of a call is
Review comment:
Added test cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1859: [CALCITE-3863] Truncate/Round return type inference through rel data …
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1859: [CALCITE-3863] Truncate/Round return type inference through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r394091663
##########
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##########
@@ -557,6 +557,32 @@ public int size() {
public static final SqlReturnTypeInference NULLABLE_MOD =
chain(DECIMAL_MOD_NULLABLE, ARG1_NULLABLE);
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE = opBinding -> {
+ RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+ RelDataType type1 = opBinding.getOperandType(0);
+ Integer scale = 0;
+ if (opBinding.getOperandCount() > 1) {
+ scale = opBinding.getOperandLiteralValue(1, Integer.class);
+ }
+ return typeFactory.getTypeSystem().deriveDecimalTruncateType(typeFactory, type1, scale);
+ };
+
+ /**
+ * Type-inference strategy whereby the result type of a call is the decimal
+ * truncate of two exact numeric operands where at least one of the operands is a
+ * decimal.
+ */
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE_NULLABLE =
+ cascade(DECIMAL_TRUNCATE, SqlTypeTransforms.TO_NULLABLE);
+
+ /**
+ * Type-inference strategy whereby the result type of a call is
Review comment:
Could you please add some test cases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397681534
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
Review comment:
I know SQL standard document the general operators (`+`, `-`, `*`, `/`, `%`) for decimal types. But can we find the guideline in SQL standard for `truncate` and `round`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] praveenbingo commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
praveenbingo commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-604246510
> > @danny0405 This is similar to #1311 which made some of the other decimal related rules configurable using a custom type system.
> > Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same.
>
> In my opinion, #1311 is very different because `+` and `-` are builtin operators and we have no way for downstream projects to replace these operators, but `Round` and `Truncate` are different, they are sql functions, and if your project has maintained the sql operators by yourself, there is a chance you just override the type inference of these two instead of the whole type system, which seems too heavy and special to customize.
Hi Danny,
Thanks for the comments. I think we discussed something similar in this comment in 1311(https://issues.apache.org/jira/browse/CALCITE-3187?focusedCommentId=16882084&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16882084).
I think the same holds true even now, I think we want to consistently override return type inference for all decimal functions so that even Calcite internals use the same rules as us. Having two sets of rules applied inconsistently between Calcite internals and Application internals will be hard to debug.
Please let us know if you have an alternative that keeps the rules consistent across the board.
Thx.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603267869
> I'm confused by your change, if you just want the `Round` or `Truncate` return type inference can be configurable, just replace the `Return type inference`, which is already pluggable.
This change preserves the existing behaviour and enables to override the behaviour if needed, using RelDataTypeFactory for a custom type system.
The test cases _testDecimalTruncateReturnTypeInference_ and _testCustomDecimalTruncateReturnTypeInference_ demonstrates the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r396947719
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,103 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Let p1, s1 be the precision and scale of the first operand</li>
+ * <li>Let s2 be the scale value to truncate</li>
+ * <li>Let p, s be the precision and scale of the result</li>
+ * <li>Let d be the number of whole digits in the result</li>
+ * <li>Then the result type is a decimal with:
+ * <ul>
+ * <li>s = s2</li>
+ * <li>p = p1</li>
+ * </ul>
+ * </li>
+ * <li>p and s are capped at their maximum values</li>
+ * </ul>
+ *
+ * @param typeFactory TypeFactory used to create output type
+ * @param type1 Type of the first operand
+ * @param scale2 Scale value to truncate to
+ * @return Result type for a decimal truncate
+ */
+ default RelDataType deriveDecimalTruncateType(RelDataTypeFactory typeFactory,
+ RelDataType type1, Integer scale2) {
+ if (SqlTypeUtil.isExactNumeric(type1)) {
+ if (SqlTypeUtil.isDecimal(type1)) {
+ // Java numeric will always have invalid precision/scale,
Review comment:
The `scale2` is never used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r396946347
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,103 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Let p1, s1 be the precision and scale of the first operand</li>
+ * <li>Let s2 be the scale value to truncate</li>
+ * <li>Let p, s be the precision and scale of the result</li>
+ * <li>Let d be the number of whole digits in the result</li>
+ * <li>Then the result type is a decimal with:
Review comment:
Only `p` `s` `p1` `s2` are used, remove the useless 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash opened a new pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash opened a new pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859
…type factory
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397193536
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,103 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Let p1, s1 be the precision and scale of the first operand</li>
+ * <li>Let s2 be the scale value to truncate</li>
+ * <li>Let p, s be the precision and scale of the result</li>
+ * <li>Let d be the number of whole digits in the result</li>
+ * <li>Then the result type is a decimal with:
+ * <ul>
+ * <li>s = s2</li>
+ * <li>p = p1</li>
+ * </ul>
+ * </li>
+ * <li>p and s are capped at their maximum values</li>
+ * </ul>
+ *
+ * @param typeFactory TypeFactory used to create output type
+ * @param type1 Type of the first operand
+ * @param scale2 Scale value to truncate to
+ * @return Result type for a decimal truncate
+ */
+ default RelDataType deriveDecimalTruncateType(RelDataTypeFactory typeFactory,
+ RelDataType type1, Integer scale2) {
+ if (SqlTypeUtil.isExactNumeric(type1)) {
+ if (SqlTypeUtil.isDecimal(type1)) {
+ // Java numeric will always have invalid precision/scale,
Review comment:
This enables to override the behaviour, where scale2 can also used. Since existing behaviour is to have return type as type1, preserving the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r396948646
##########
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##########
@@ -557,6 +557,55 @@ public int size() {
public static final SqlReturnTypeInference NULLABLE_MOD =
chain(DECIMAL_MOD_NULLABLE, ARG1_NULLABLE);
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE = opBinding -> {
+ RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+ RelDataType type1 = opBinding.getOperandType(0);
+ // passing random value for scale2
+ // since the default behaviour is to have the return type of result as the first operand
+ return typeFactory.getTypeSystem().deriveDecimalTruncateType(typeFactory, type1, 0);
+ };
Review comment:
just return `type1` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603264439
> I'm confused by your change, if you just want the `Round` or `Truncate` return type inference can be configurable, just replace the `Return type inference`, which is already pluggable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] praveenbingo commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
praveenbingo commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397710842
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
Review comment:
There is not a consistent one from what we found SQL Server seems to go with what calcite does where as Oracle does not.
Also i think that is outside 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603748315
> @danny0405 This is similar to #1311 which made some of the other decimal related rules configurable using a custom type system.
>
> Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same.
In my opinion, #1311 is very different because `+` and `-` are builtin operators and we have no way for downstream projects to replace these operators, but `Round` and `Truncate` is different, they are sql functions, and if your project have maintained the sql operators by yourself, there is a chance you just override the type inference of these two instead of the whole type system, which seems too heavy and special to customize.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] praveenbingo commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
praveenbingo commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#issuecomment-603368191
@danny0405 This is similar to https://github.com/apache/calcite/pull/1311/ which made some of the other decimal related rules configurable using a custom type system.
Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397182890
##########
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##########
@@ -557,6 +557,55 @@ public int size() {
public static final SqlReturnTypeInference NULLABLE_MOD =
chain(DECIMAL_MOD_NULLABLE, ARG1_NULLABLE);
+ public static final SqlReturnTypeInference DECIMAL_TRUNCATE = opBinding -> {
+ RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
+ RelDataType type1 = opBinding.getOperandType(0);
+ // passing random value for scale2
+ // since the default behaviour is to have the return type of result as the first operand
+ return typeFactory.getTypeSystem().deriveDecimalTruncateType(typeFactory, type1, 0);
+ };
Review comment:
This enables to override the behaviour if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] anilbash closed pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
anilbash closed pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
URL: https://github.com/apache/calcite/pull/1859#discussion_r397756517
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##########
@@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory,
return null;
}
+ /**
+ * Infers the return type of a decimal truncate operation. Decimal truncate
+ * involves at least one decimal operand.
+ *
+ * <p>The default implementation is SQL:2003 compliant: the declared type of
+ * the result is the declared type of the first operand (decimal to be truncated).
+ *
+ * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
Review comment:
My point is the document added in this PR. If it is not a standard manner, we'd better not add this confusing paragraph.
> The default implementation is SQL:2003 compliant: the declared type of
> the result is the declared type of the first operand (decimal to be truncated).
> @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services