[CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … #1859

anilbash · 2020-03-17T18:15:26Z

…type factory

chunweilei · 2020-03-18T03:45:55Z

core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java

+      cascade(DECIMAL_TRUNCATE, SqlTypeTransforms.TO_NULLABLE);
+
+  /**
+   * Type-inference strategy whereby the result type of a call is


Could you please add some test cases?

Added test cases.

chunweilei

The title is a little confusing. Could you improve it?

…type factory

danny0405 · 2020-03-24T07:28:23Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

+   * <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:


Only p s p1 s2 are used, remove the useless comments.

removed done.

danny0405 · 2020-03-24T07:31:48Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

+      RelDataType type1, Integer scale2) {
+    if (SqlTypeUtil.isExactNumeric(type1)) {
+      if (SqlTypeUtil.isDecimal(type1)) {
+        // Java numeric will always have invalid precision/scale,


The scale2 is never used.

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 enables to override the behaviour, where scale2 can also used. Since existing behaviour is to have return type as type1, preserving the same.

danny0405 · 2020-03-24T07:33:54Z

core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java

+    // 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);
+  };


just return type1 ?

This enables to override the behaviour if needed.

danny0405 · 2020-03-24T07:38:08Z

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.

…type factory

anilbash · 2020-03-24T14:22:29Z

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.

praveenbingo · 2020-03-24T16:57:17Z

@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.

DonnyZone · 2020-03-25T08:34:28Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java

+   * <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


I know SQL standard document the general operators (+, -, *, /, %) for decimal types. But can we find the guideline in SQL standard for truncate and round?

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..

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

danny0405 · 2020-03-25T09:56:57Z

@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.

praveenbingo · 2020-03-26T06:01:51Z

@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.

…type factory

[CALCITE-3863] Truncate/Round return type inference through rel data …

893505d

…type factory

chunweilei reviewed Mar 18, 2020

View reviewed changes

anilbash changed the title ~~[CALCITE-3863] Truncate/Round return type inference through rel data …~~ [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … Mar 18, 2020

anilbash added 3 commits March 18, 2020 17:10

[CALCITE-3863] Truncate/Round return type inference through rel data …

b8e631d

…type factory

[CALCITE-3863] Truncate/Round return type inference through rel data …

4897851

…type factory

[CALCITE-3863] Truncate/Round return type inference through rel data …

c45b409

…type factory

danny0405 reviewed Mar 24, 2020

View reviewed changes

[CALCITE-3863] Truncate/Round return type inference through rel data …

552d9ab

…type factory

anilbash closed this Mar 24, 2020

anilbash reopened this Mar 24, 2020

DonnyZone reviewed Mar 25, 2020

View reviewed changes

danny0405 added the returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR label Mar 25, 2020

anilbash added 2 commits March 28, 2020 02:49

[CALCITE-3863] Truncate/Round return type inference through rel data …

da4b135

…type factory

[CALCITE-3863] Truncate/Round return type inference through rel data …

459a05a

…type factory

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:52

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:19

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … #1859

[CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … #1859

anilbash commented Mar 17, 2020

chunweilei Mar 18, 2020

anilbash Mar 18, 2020

chunweilei left a comment

danny0405 Mar 24, 2020

anilbash Mar 24, 2020

danny0405 Mar 24, 2020

danny0405 Mar 24, 2020

anilbash Mar 24, 2020

danny0405 Mar 24, 2020

anilbash Mar 24, 2020

danny0405 commented Mar 24, 2020

anilbash commented Mar 24, 2020

praveenbingo commented Mar 24, 2020

DonnyZone Mar 25, 2020

praveenbingo Mar 25, 2020

DonnyZone Mar 25, 2020

danny0405 commented Mar 25, 2020 •

edited

Loading

praveenbingo commented Mar 26, 2020

[CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … #1859

Are you sure you want to change the base?

[CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … #1859

Conversation

anilbash commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunweilei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Mar 24, 2020

anilbash commented Mar 24, 2020

praveenbingo commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 commented Mar 25, 2020 • edited Loading

praveenbingo commented Mar 26, 2020

danny0405 commented Mar 25, 2020 •

edited

Loading