Re: Avoid extra Sort nodes between WindowAggs when sorting can be reused

On Tue, Jul 3, 2018 at 6:19 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
>> On 2 Jul 2018, at 14:01, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
>> Thank you for updating the patch! There are two review comments.
>
> Thanks for reviewing!
>
>> The current select_active_windows() function compares the all fields
>> of WindowClause for the sorting but with this patch we compare only
>> tleSortGroupRef, sortop and the number of uniqueOrder. I think this
>> leads a degradation as follows.
>
> You are right, that was an oversight.  The attached patch takes a stab at
> fixing this.
>
>> s/readibility/readability/
>
> Fixed.

Thank you for updating the patch.

+               if (sca->tleSortGroupRef > scb->tleSortGroupRef)
+                       return -1;
+               else if (sca->tleSortGroupRef < scb->tleSortGroupRef)
+                       return 1;
+               else if (sca->sortop > scb->sortop)
+                       return -1;
+               else if (sca->sortop < scb->sortop)
+                       return 1;
+               else if (sca->nulls_first && !scb->nulls_first)
+                       return -1;
+               else if (!sca->nulls_first && scb->nulls_first)
+                       return 1;

Hmm, this is missing the eqop fields of SortGroupClause. I haven't
tested yet but does the similar degradation happen if two
SortGroupCaluses have different eqop and the same other values?

--
The source code comments for common_prefix_cmp() function and
WindowClauseSortNode struct is needed.

--
+-- Test Sort node reordering
+EXPLAIN (COSTS OFF)
+SELECT
+  lead(1) OVER (PARTITION BY depname ORDER BY salary, enroll_date),
+  lag(1) OVER (PARTITION BY depname ORDER BY salary,enroll_date,empno)
+  from empsalary;

I think it's better to change "from empsalary" to "FROM empsalary" for
consistency with other code.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center