On Tue, Jul 3, 2018 at 6:19 AM, Daniel Gustafsson <dan...@yesql.se> wrote: >> On 2 Jul 2018, at 14:01, Masahiko Sawada <sawada.m...@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
