On Tue, Jun 4, 2024 at 1:47 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Mon, Jun 3, 2024 at 6:55 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > On Sun, 2 Jun 2024 at 17:18, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >> On Sun, Jun 2, 2024 at 10:55 AM jian he <jian.universality@gmail.com> wrote:
> >> >
> >> > On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov <aekorotkov@gmail.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
EquivalenceClassunless 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'tchecked the comments provided they are restored from pre 0452b461bc state.
> >
> > 0005: Looks right.
>
> Thank you. Revised according to your comments. I think 0001-0004
> comprising a good refactoring addressing concerns from Tom [1].
> 0001 Removes from get_eclass_for_sort_expr() unnatural responsibility
> of setting EquivalenceClass.ec_sortref. Instead this field is set in
> make_pathkeys_for_sortclauses_extended() while processing of group
> clauses.
> 0002 Provides additional asserts. That should helping to defend
> against lurking bugs.
> 0003 Fixes unsuitable name of data structure.
> 0004 Restores pre 0452b461bc state to preprocess_groupclause() and
> lowers the number of paths to consider.
> It would be good to explicitly hear Tom about this. But I'm quite
> sure these patches makes situation better not worse. I'm going to
> push them if no objections.
>
> I'm putting 0005 aside. It's unclear why and how there could be
> duplicate SortGroupClauses at that stage. Also, it's unclear why
> existing code handles them bad. So, this should wait a comment from
> Tom.
>
> Links.
> 1. https://www.postgresql.org/message-id/266850.1712879082%40sss.pgh.pa.us
Ops... I found I've published an old version of patchset. The
relevant version is attached.
------
Regards,
Alexander Korotkov
Supabase