calcite icon indicating copy to clipboard operation
calcite copied to clipboard

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

Open anilbash opened this issue 5 years ago • 5 comments

…type factory

anilbash avatar Mar 17 '20 18:03 anilbash

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.

danny0405 avatar Mar 24 '20 07:03 danny0405

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.

anilbash avatar Mar 24 '20 14:03 anilbash

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

praveenbingo avatar Mar 24 '20 16:03 praveenbingo

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

danny0405 avatar Mar 25 '20 09:03 danny0405

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

praveenbingo avatar Mar 26 '20 06:03 praveenbingo