|From:||"Hou, Zhijie" <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>|
|To:||Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||RE: Add Nullif case for eval_const_expressions_mutator|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
> Check your whitespace usage:
> 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
> 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;
Attatching v3 patch, please consider it for further review.
|Next Messageemail@example.com||2021-01-20 01:32:47||RE: Parallel INSERT (INTO ... SELECT ...)|
|Previous Message||Peter Geoghegan||2021-01-20 01:14:25||Re: New IndexAM API controlling index vacuum strategies|