[CALCITE-7562] SqlToRel misses CAST in case IN expression without type coercion#4972
Conversation
| * and the IN-to-OR expansion produces comparisons with mismatched types | ||
| * (e.g., DATE = CHAR). | ||
| */ | ||
| private RexNode ensureComparisonTypes(RexNode node) { |
There was a problem hiding this comment.
does this produce the same result as type coercion would?
Why not the leastRestrictive type?
There was a problem hiding this comment.
yes, same result, before the fix it didn't
Why not the leastRestrictive type?
- follow the approach that was before CALCITE-6435
- in case of no leastRestrictive it will fail with NPE which is not user friednly
in case of cast failure the error message will contain more helpful error description
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
I actually like type coercion, I don't understand why you would have NO type coercion.
Skipping type coercion makes it much more complicated to understand the semantics of the program. The validator makes some assumptions about what programs are legal and why, and the consumers of the program produced by the validator may make different assumptions (e.g., do you cast int to char or vice-versa when comparing the two?). With explicit casts everywhere there are no assumptions.
This PR introduces a little more coercion even when the users ask for no coercion, so I think it's good.
mihaibudiu
added
the
LGTM-will-merge-soon
Looks like historical reasoning, there is still a number of tests in Flink failing if I blindly enable them. So for now enabling coercion in Flink requires discuss in ML first and may be fixing all the failing cases which is in todo list however not with high priority |
|
rebased to the latest main |
|
|
I approved, feel free to merge anytime |
2 participants
Jira Link
CALCITE-7562
Changes Proposed
The PRs adds back (was removed at CALCITE-6435)
ensureTypeforINin case of different type family and turned off type coercion