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.
`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".
I did some minor cosmetic changes, mainly changing foreach to foreach_node.
Please check the attachment.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index d6a1fb8e..801d9f6d 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -2852,15 +2852,13 @@ preprocess_groupclause(PlannerInfo *root, List *force)
{
Query *parse = root->parse;
List *new_groupclause = NIL;
- ListCell *sl;
- ListCell *gl;
+ ListCell *gl;
/* For grouping sets, we need to force the ordering */
- if (force)
+ if (force != NIL)
{
- foreach(sl, force)
+ foreach_int(ref, force)
{
- Index ref = lfirst_int(sl);
SortGroupClause *cl = get_sortgroupref_clause(ref, parse->groupClause);
new_groupclause = lappend(new_groupclause, cl);
@@ -2879,10 +2877,8 @@ preprocess_groupclause(PlannerInfo *root, List *force)
*
* This code assumes that the sortClause contains no duplicate items.
*/
- foreach(sl, parse->sortClause)
+ foreach_node(SortGroupClause, sc, parse->sortClause)
{
- SortGroupClause *sc = lfirst_node(SortGroupClause, sl);
-
foreach(gl, parse->groupClause)
{
SortGroupClause *gc = lfirst_node(SortGroupClause, gl);
@@ -2908,10 +2904,8 @@ preprocess_groupclause(PlannerInfo *root, List *force)
* implemented using incremental sort. Also, give up if there are any
* non-sortable GROUP BY items, since then there's no hope anyway.
*/
- foreach(gl, parse->groupClause)
+ foreach_node(SortGroupClause,gc, parse->groupClause)
{
- SortGroupClause *gc = lfirst_node(SortGroupClause, gl);
-
if (list_member_ptr(new_groupclause, gc))
continue; /* it matched an ORDER BY item */
if (!OidIsValid(gc->sortop)) /* give up, GROUP BY can't be sorted */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index af8de421..2ba297c1 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -426,7 +426,7 @@ struct PlannerInfo
* items to be proven redundant, implying that there is only one group
* containing all the query's rows. Hence, if you want to check whether
* GROUP BY was specified, test for nonempty parse->groupClause, not for
- * nonempty processed_groupClause. Optimiser chooses specific order of
+ * nonempty processed_groupClause. Optimizer chooses specific order of
* group-by clauses during the upper paths generation process, attempting
* to use different strategies to minimize number of sorts or engage
* incremental sort. See preprocess_groupclause() and
