[CALCITE-7438] Do not rewrite NULLIF to a CASE expression#4837
[CALCITE-7438] Do not rewrite NULLIF to a CASE expression#4837cjj2010 wants to merge 5 commits intoapache:mainfrom
Conversation
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
LGTM!
I only left some comments on coding style.
| .ok(expected2); | ||
| } | ||
|
|
||
| @Test void testNullIfAgg() { |
There was a problem hiding this comment.
Could these cases be combined into one? Just like:
SELECT NULLIF(COUNT(brand_name),1), NULLIF(COUNT(brand_name) + 1,1), NULLIF(cases_per_pallet + 1,1) FROM product;
Also you need to add a jira link in javadoc.
| /** | ||
| * The <code>NULLIF</code> function. | ||
| */ | ||
| public class SqlNullifFunction extends SqlFunction { |
There was a problem hiding this comment.
This Function should to be marked as deprecated using @deprecated.
There was a problem hiding this comment.
@cjj2010 the PR was veto-ed by @julianhyde
You have to make an argument about why this is needed.
Perhaps the JIRA case is the right place for this discussion.
There was a problem hiding this comment.
Yes, we need to back to JIRA to continue the discussion. Perhaps you need to answer Julian's question first.
There was a problem hiding this comment.
Okay, I have already replied to Jira. From the current situation, Calcite will directly convert nullif to casewhen by default. I think there are two solutions at present: 1. If there is an aggregation function, it will not be converted to casewhen; 2. Keep nullif in all cases and add optimization rules, such as NULLIF(x + 1, 5), we use common subexpression elimination so that the argument "x + 1" is only evaluated once @mihaibudiu @xiedeyantu
|
|
||
| defineMethod(AGE, BuiltInMethod.AGE.method, NullPolicy.STRICT); | ||
|
|
||
| define(NULLIF, new NullifImplementor()); |
There was a problem hiding this comment.
Please move this definition below IS_DISTINCT_FROM?
|
Some ideas, but not related to this PR. Perhaps we can consider simplifying NULLIF in some specific cases. Because currently NULLIF can be simplified to CASE WHEN, I'm not sure whether it can be simplified or not. |
|
-1 The commits look fine, but per the Jira discussion, the current behavior is fine and there’s no need to fix it. |
|
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
I feel that you didn't seem to directly answer Julian's question; he likely hasn't approved your changes yet.
4 participants
Jira:https://issues.apache.org/jira/browse/CALCITE-7438