On 2021-01-12 07:43, Hou, Zhijie wrote:
>>> I think this patch should be about a tenth the size. Try modeling it
>>> on the T_SubscriptingRef-etc case, ie, use ece_generic_processing and
>>> then ece_evaluate_expr to cover the generic cases. OpExpr is common
>>> enough to deserve specially optimized code, but NullIf isn't, so shorter
>> is better.
>>
>> Thanks for the review.
>>
>> Attaching v2 patch , which followed the suggestion to use
>> ece_generic_processing and ece_evaluate_expr to simplify the code.
>>
>> Please have a check.
>
> Sorry, I found the code still be simplified better.
> Attaching the new patch.
It's a bit unfortunate now that between OpExpr, DistinctExpr,
NullIfExpr, and to a lesser extent ScalarArrayOpExpr we will now have
several different implementations of nearly the same thing, without any
explanation why one approach was chosen here and another there. We
should at least document this.
Some inconsistencies I found: The code for DistinctExpr calls
expression_tree_mutator() directly, but your code for NullIfExpr calls
ece_generic_processing(), even though the explanation in the comment for
DistinctExpr would apply there as well.
Your code for NullIfExpr doesn't appear to call set_opfuncid() anywhere.
I would move your new block for NullIfExpr after the block for
DistinctExpr. That's the order in which these blocks appear elsewhere
for generic node processing.
Check your whitespace usage:
if(!has_nonconst_input)
should have a space after the "if". (It's easy to fix of course, but I
figure I'd point it out here since you have submitted several patches
with this style, so it's perhaps a habit to break.)
Perhaps add a comment to the tests like this so it's clear what they are
for:
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 4742e1d0e0..98e3fb8de5 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -137,6 +137,7 @@ CREATE TABLE CASE2_TBL (
FROM CASE_TBL a, CASE2_TBL b
WHERE COALESCE(f,b.i) = 2;
+-- Tests for constant subexpression simplification
explain (costs off)
SELECT * FROM CASE_TBL WHERE NULLIF(1, 2) = 2;
