Hi
Thanks for the review.
> 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.
I am not quiet sure where to document the difference.
Temporarily, I tried to add some comments for the Nullif to explain why this one is different.
+ /*
+ * Since NullIf is not common enough to deserve specially
+ * optimized code, use ece_generic_processing and
+ * ece_evaluate_expr to simplify the code as much as possible.
+ */
Any suggestions ?
> 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.
IMO, we will call set_opfuncid in function ece_evaluate_expr.
Like the following flow:
ece_evaluate_expr-->evaluate_expr--> fix_opfuncids--> fix_opfuncids_walker--> set_opfuncid
And we do not need the opfuncid till we call ece_evaluate_expr.
So, to simplify the code as much as possible, I did not call set_opfuncid in eval_const_expressions_mutator again.
> 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.
>
Changed.
> 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.)
Changed.
> 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;
Added.
Attatching v3 patch, please consider it for further review.
Best regards,
houzj
