Re: Add proper planner support for ORDER BY / DISTINCT aggregates

Em ter., 13 de jul. de 2021 às 01:44, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> Thanks for having a look at this.
>
> On Tue, 13 Jul 2021 at 11:04, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> >> 0001 Adds planner support for ORDER BY aggregates.
> >
> > /* Normal transition function without ORDER BY / DISTINCT. */
> > Is it possible to avoid entering to initialize args if 'argno >=
> pertrans->numTransInputs'?
> > Like this:
> >
> > if (!pertrans->aggsortrequired && argno < pertrans->numTransInputs)
> >
> > And if argos is '>' that pertrans->numTransInputs?
> > The test shouldn't be, inside the loop?
> >
> > + /*
> > + * Don't initialize args for any ORDER BY clause that might
> > + * exist in a presorted aggregate.
> > + */
> > + if (argno >= pertrans->numTransInputs)
> > + break;
>
> The idea is to stop the loop before processing any Aggref arguments
> that might belong to the ORDER BY clause.

Yes, I get the idea.

We must still process other
> arguments up to the ORDER BY args though,

I may have misunderstood, but the other arguments are under
pertrans->numTransInputs?

> so we can't skip this loop.
>
The question not answered is if *argno* can '>=' that
pertrans->numTransInputs,
before entering the loop?
If *can*, the loop might be useless in that case.

>
> Note that we're doing argno++ inside the loop.

Another question is, if *argno* can '>' that pertrans->numTransInputs,
before the loop, the test will fail?
if (argno == pertrans->numTransInputs)

>
> > I think that or can reduce the scope of variable 'sortlist' or simply
> remove it?
>
> I've adjusted the scope of this. I didn't want to remove it because
> it's kinda useful to have it that way otherwise the 0002 patch would
> need to add it.
>
Nice.

> >> 0002 is a WIP patch for DISTINCT support. This still lacks JIT
> >> support and I'm still not certain of the best where to store the
> >> previous value or tuple to determine if the current one is distinct
> >> from it.
> >
> > In the patch 0002, I think that can reduce the scope of variable
> 'aggstate'?
> >
> > + EEO_CASE(EEOP_AGG_PRESORTED_DISTINCT_SINGLE)
>
> Yeah, that could be done.
>
> I've attached the updated patches.
>
Thanks.

regards,
Ranier Vilela