Hi, Alexander! On Sun, 2 Jun 2024 at 17:18, Alexander Korotkov <aekorot...@gmail.com> wrote:
> Hi! > > On Sun, Jun 2, 2024 at 10:55 AM jian he <jian.universal...@gmail.com> > wrote: > > > > On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov <aekorot...@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 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
