Re: Re: FETCH FIRST clause WITH TIES option

On Sun, Mar 31, 2019 at 3:14 AM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

>
>
> Hi,
>
> I got to look at the patch today, with the intent to commit, but sadly I
> ran into a couple of minor issues that I don't feel comfortable fixing
> on my own. Attached is a patch highlighling some of the places (0001 is
> your v7 patch, to keep the cfbot happy).
>
>
Thank you

>
> 1) the docs documented this as
>
> ... [ ONLY | WITH TIES ]
>
> but that's wrong, because it implies those options are optional (i.e.
> the user may not specify anything). That's not the case, exactly one
> of those options needs to be specified, so it should have been
>
> ... { ONLY | WITH TIES }
>
>
> 2) The comment in ExecLimit() needs to be updated to explain that WITH
> TIES changes the behavior.
>
>
> 3) Minor code style issues (no space before * on comment lines, {}
> around single-line if statements, ...).
>
>
> 4) The ExecLimit() does this
>
> if (node->limitOption == WITH_TIES)
> ExecCopySlot(node->last_slot, slot);
>
> but I think we only really need to do that for the last tuple in the
> window, no? Would it be a useful optimization?
>
>
>
I think it is good optimization .Fixed

5) Two issues in _outLimit(). Firstly, when printing uniqCollations the
> code actually prints uniqOperators. Secondly, why does the code use
> these loops at all, instead of using WRITE_ATTRNUMBER_ARRAY and
> WRITE_OID_ARRAY, like other places? Perhaps there's an issue with empty
> arrays? I haven't tested this, but looking at the READ_ counterparts, I
> don't see why that would be the case.
>
>
>
Fixed

regards
Surafel