Horton

Thread

Commits

  1. Add support for NullIfExpr in eval_const_expressions

  1. Add Nullif case for eval_const_expressions_mutator

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-11-10T11:31:47Z

    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

  2. Re: Add Nullif case for eval_const_expressions_mutator

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-11T16:20:53Z

    "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

  3. RE: Add Nullif case for eval_const_expressions_mutator

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2021-01-12T04:58:34Z

    > > 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

  4. RE: Add Nullif case for eval_const_expressions_mutator

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2021-01-12T06:43:09Z

    > > 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

  5. Re: Add Nullif case for eval_const_expressions_mutator

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-01-15T06:59:28Z

    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;

  6. RE: Add Nullif case for eval_const_expressions_mutator

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2021-01-20T01:16:38Z

    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

  7. Re: Add Nullif case for eval_const_expressions_mutator

    David Steele <david@pgmasters.net> — 2021-03-23T14:31:46Z

    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

  8. Re: Add Nullif case for eval_const_expressions_mutator

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T15:45:37Z

    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

  9. RE: Add Nullif case for eval_const_expressions_mutator

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-03-24T10:52:04Z

    > +				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

  10. Re: Add Nullif case for eval_const_expressions_mutator

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-30T09:20:04Z

    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.

  11. Re: Add Nullif case for eval_const_expressions_mutator

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-04-02T09:08:34Z

    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