Re: POC: GROUP BY optimization

Hi, Alexander!

On Sun, 2 Jun 2024 at 17:18, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> Hi!
>
> On Sun, Jun 2, 2024 at 10:55 AM jian he <jian(dot)universality(at)gmail(dot)com>
> wrote:
> >
> > On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
> wrote:
> > >
> > > I've revised some grammar including the sentence you've proposed.
> > >
> >
> > -static List *groupclause_apply_groupingset(PlannerInfo *root, List
> *force);
> > +static List *preprocess_groupclause(PlannerInfo *root, List *force);
> >
> > changing preprocess_groupclause the second argument
> > from "force" to "gset" would be more intuitive, I think.
>
> Probably, but my intention is to restore preprocess_groupclause() as
> it was before 0452b461bc with minimal edits to support incremental
> sort. I'd rather avoid refactoring if this area for now.
>
> > `elog(ERROR, "Order of group-by clauses doesn't correspond incoming
> > sort order");`
> >
> > I think this error message makes people wonder what "incoming sort
> order" is.
> > BTW, "correspond", generally people use "correspond to".
>
> Thank you. On the second thought, I think it would be better to turn
> this into an assertion like the checks before.
>
> > I did some minor cosmetic changes, mainly changing foreach to
> foreach_node.
> > Please check the attachment.
>
> I would avoid refactoring of preprocess_groupclause() for the reason
> described above. But I picked the grammar fix for PlannerInfo's
> comment.
>

Thank you for working on this patchset.

0001: Patch looks right

Comments:

Covert -> Convert
sets the uninitialized value of ec_sortref of the pathkey's
EquivalenceClass -> sets the value of the pathkey's EquivalenceClass unless
it's already initialized
wasn't set yet -> hasn't been set yet

0002: additional assert checking only. Looks right.

0003: pure renaming, looks good.

0004: Restores pre 0452b461bc state to preprocess_groupclause with removed
partial_match fallback. Looks right. I haven't checked the comments
provided they are restored from pre 0452b461bc state.

0005: Looks right.

Regards,
Pavel Borisov
Supabase