Em ter., 13 de jul. de 2021 às 01:44, David Rowley <dgrowle...@gmail.com> escreveu:
> Thanks for having a look at this. > > On Tue, 13 Jul 2021 at 11:04, Ranier Vilela <ranier...@gmail.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
