Thread: Add Nullif case for eval_const_expressions_mutator
Hi I notice that there are no Nullif case in eval_const_expression. Since Nullif is similar to Opexpr and is easy to implement, I try add this case in eval_const_expressions_mutator. Best regards, houzj
Attachment
"Hou, Zhijie" <houzj.fnst@cn.fujitsu.com> writes:
> I notice that there are no Nullif case in eval_const_expression.
> Since Nullif is similar to Opexpr and is easy to implement,
> I try add this case in eval_const_expressions_mutator.
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.
regards, tom lane
> > I notice that there are no Nullif case in eval_const_expression. > > Since Nullif is similar to Opexpr and is easy to implement, I try add > > this case in eval_const_expressions_mutator. > > 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. Best regards, houzj
Attachment
> > 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. Best regards, houzj
Attachment
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;
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
Attachment
On 1/19/21 8:16 PM, Hou, Zhijie wrote: > > Attatching v3 patch, please consider it for further review. Peter, thoughts on the new patch in [1]? -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/ab53b3dbdbd6436f970f33b51ccd7dd3%40G08CNEXMBPEKD05.g08.fujitsu.local
David Steele <david@pgmasters.net> writes:
> Peter, thoughts on the new patch in [1]?
I'm not Peter, but I have a complaint about this bit:
+ if (!has_nonconst_input)
+ return ece_evaluate_expr(expr);
That's not okay without a further check to see if the comparison function
used by the node is immutable. Compare ScalarArrayOpExpr, for instance.
regards, tom lane
> + if (!has_nonconst_input) > + return ece_evaluate_expr(expr); > > That's not okay without a further check to see if the comparison function used > by the node is immutable. Compare ScalarArrayOpExpr, for instance. Thanks for pointing it out. Attaching new patch with this change. Best regards, houzj
Attachment
On 24.03.21 11:52, houzj.fnst@fujitsu.com wrote: >> + if (!has_nonconst_input) >> + return ece_evaluate_expr(expr); >> >> That's not okay without a further check to see if the comparison function used >> by the node is immutable. Compare ScalarArrayOpExpr, for instance. > > Thanks for pointing it out. > Attaching new patch with this change. This patch looks okay to me and addresses all the feedback that was given. If there are no more comments, I'll commit it in a few days.
On 30.03.21 11:20, Peter Eisentraut wrote: > On 24.03.21 11:52, houzj.fnst@fujitsu.com wrote: >>> + if (!has_nonconst_input) >>> + return ece_evaluate_expr(expr); >>> >>> That's not okay without a further check to see if the comparison >>> function used >>> by the node is immutable. Compare ScalarArrayOpExpr, for instance. >> >> Thanks for pointing it out. >> Attaching new patch with this change. > > This patch looks okay to me and addresses all the feedback that was > given. If there are no more comments, I'll commit it in a few days. done