[CALCITE-6091] Char that in array or map is truncated if CASE WHEN statement contains null #3501
Conversation
JiajunBernoulli
force-pushed
the
CALCITE-6091
branch
from
88b9a85 to
f7bfe92
Compare
JiajunBernoulli
force-pushed
the
CALCITE-6091
branch
from
f7bfe92 to
9a560ac
Compare
| * <a href="https://issues.apache.org/jira/browse/CALCITE-6091">[CALCITE-6091] | ||
| * Char that in array or map is truncated in CASE WHEN statement</a>. | ||
| */ | ||
| @Test void testTruncatedCharInCaseWhen() { |
There was a problem hiding this comment.
I don't understand how this test relates to the change in the code.
The code is about nullability, but this test does not have any null values.
But the issue is about the type inference across cases in a case statement.
This would suggest that the bug is actually in leastRestrictive, which doesn't handle properly nullable types?
There was a problem hiding this comment.
@mihaibudiu , Thanks for your review.
I change the JIRA summary because https://issues.apache.org/jira/browse/CALCITE-4603 has been resolved.
I just add the case if types has null.
There was a problem hiding this comment.
Where do these new tests use null values?
Will these tests pass if you remove your fixes? If yes, then these tests are not sufficient.
…atement contains null
JiajunBernoulli
changed the title
|
Please retry analysis of this Pull-Request directly on SonarCloud. |
| fixture.check("select case " | ||
| + "when true then array('abc') " | ||
| + "when false then array('d') " | ||
| + "else null " |
There was a problem hiding this comment.
@mihaibudiu
Here is null value.
When we omit
else, it is also null.
If I remove my fix, the test will failed.
There was a problem hiding this comment.
The type produced CHAR(3) NOT NULL ARRAY suggests that the expression is simplified before type inference.
If the expression wasn't simplified the type should be VARCHAR ARRAY.
But probably the simplification is done after type inference, so that's why this test works.
| fixture.check("select case " | ||
| + "when true then array('abc') " | ||
| + "when false then array('d') " | ||
| + "else null " |
There was a problem hiding this comment.
The type produced CHAR(3) NOT NULL ARRAY suggests that the expression is simplified before type inference.
If the expression wasn't simplified the type should be VARCHAR ARRAY.
But probably the simplification is done after type inference, so that's why this test works.
| isNullable |= type.isNullable(); | ||
| } | ||
| final RelDataType type = | ||
| leastRestrictive( | ||
| Util.transform(types, | ||
| Util.transform(notNullTypes, |
There was a problem hiding this comment.
My feeling was that leastRestrictive should be fixed instead to deal correctly with the NULL type.
So this solution is fine, but if leastRestrictive can be made more general, other users could benefit.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
No description provided.