Re: Case expression pushdown

Le 22/06/2021 à 15:39, Alexander Pyhalov a écrit :
> Seino Yuki писал 2021-06-22 16:03:
>> On 2021-06-16 01:29, Alexander Pyhalov wrote:
>>> Hi.
>>>
>>> Ashutosh Bapat писал 2021-06-15 16:24:
>>>> Looks quite useful to me. Can you please add this to the next 
>>>> commitfest?
>>>>
>>>
>>> Addded to commitfest. Here is an updated patch version.
>>
>> Thanks for posting the patch.
>> I agree with this content.
>>
>>> + Foreign Scan on public.ft2 (cost=156.58..165.45 rows=394 width=14)
>> It's not a big issue, but is there any intention behind the pattern of
>> outputting costs in regression tests?
>
> Hi.
>
> No, I don't think it makes much sense. Updated tests (also added case 
> with empty else).


The patch doesn't apply anymore to master, I join an update of your 
patch update in attachment. This is your patch rebased and untouched 
minus a comment in the test and renamed to v4.


I could have miss something but I don't think that additional struct 
elements case_args in structs foreign_loc_cxt and deparse_expr_cxt are 
necessary. They look to be useless.

The patch will also be more clear if the CaseWhen node was handled 
separately in foreign_expr_walker() instead of being handled in the 
T_CaseExpr case. By this way the T_CaseExpr case just need to call 
recursively foreign_expr_walker(). I also think that code in 
T_CaseTestExpr should just check the collation, there is nothing more to 
do here like you have commented the function deparseCaseTestExpr(). This 
function can be removed as it does nothing if the case_args elements are 
removed.


There is a problem the regression test with nested CASE clauses:

    EXPLAIN (VERBOSE, COSTS OFF)
    SELECT c1,c2,c3 FROM ft2 WHERE CASE CASE WHEN c2 > 0 THEN c2 END
    WHEN 100 THEN 601 WHEN c2 THEN c2 ELSE 0 END > 600 ORDER BY c1;

the original query use "WHERE CASE CASE WHEN" but the remote query is 
not the same in the plan:

    Remote SQL: SELECT "C 1", c2, c3 FROM "S 1"."T 1" WHERE (((CASE WHEN
    ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = 100) THEN 601
    WHEN ((CASE WHEN (c2 > 0) THEN c2 ELSE NULL::integer END) = c2) THEN
    c2 ELSE 0 END) > 600)) ORDER BY "C 1" ASC NULLS LAST

Here this is "WHERE (((CASE WHEN ((CASE WHEN" I expected it to be 
unchanged to "WHERE (((CASE (CASE WHEN".


Also I would like the following regression tests to be added. It test 
that the CASE clause in aggregate and function is pushed down as well as 
the aggregate function. This was the original use case that I wanted to 
fix with this feature.

    -- CASE in aggregate function, both must be pushed down
    EXPLAIN (VERBOSE, COSTS OFF)
    SELECT sum(CASE WHEN mod(c1, 4) = 0 THEN 1 ELSE 2 END) FROM ft1;
    -- Same but without the ELSE clause
    EXPLAIN (VERBOSE, COSTS OFF)
    SELECT sum(CASE WHEN mod(c1, 4) = 0 THEN 1 END) FROM ft1;


For convenience I'm attaching a new patch v5 that change the code 
following my comments above, fix the nested CASE issue and adds more 
regression tests.


Best regards,

-- 
Gilles Darold