On Sat, 2006-03-04 at 21:21 -0500, Tom Lane wrote: > 1. For each ORDER BY item: > 1a. Find a matching GROUP BY item. > (Break out of loop if no match.) > 1b. Add it to the output list with ORDER BY item's ordering op. > 1c. Remove the matched item from the GROUP BY list. > 2. For each remaining GROUP BY item: > 2a. Add it to output list using default ordering op.
Okay, attached is a revised patch that implements this. Barring any objections I'll apply it tomorrow. -Neil
============================================================ *** src/backend/parser/parse_clause.c 1b13820b23dd57bd4e00ffbd2f979015b1536243 --- src/backend/parser/parse_clause.c e7f9499effe2dbe572668c83ed455b49299954af *************** *** 1296,1333 **** return target_result; } /* * transformGroupClause - * transform a GROUP BY clause * * GROUP BY items will be added to the targetlist (as resjunk columns) * if not already present, so the targetlist must be passed by reference. */ List * transformGroupClause(ParseState *pstate, List *grouplist, List **targetlist, List *sortClause) { ! List *glist = NIL; ! ListCell *gl; ! ListCell *sortItem; ! sortItem = list_head(sortClause); ! ! foreach(gl, grouplist) { TargetEntry *tle; ! Oid restype; ! Oid ordering_op; ! GroupClause *grpcl; ! tle = findTargetlistEntry(pstate, lfirst(gl), targetlist, GROUP_CLAUSE); - /* avoid making duplicate grouplist entries */ - if (targetIsInSortList(tle, glist)) - continue; - /* if tlist item is an UNKNOWN literal, change it to TEXT */ restype = exprType((Node *) tle->expr); --- 1296,1341 ---- return target_result; } + static GroupClause * + make_group_clause(TargetEntry *tle, List *targetlist, Oid sortop) + { + GroupClause *result; + result = makeNode(GroupClause); + result->tleSortGroupRef = assignSortGroupRef(tle, targetlist); + result->sortop = sortop; + return result; + } + /* * transformGroupClause - * transform a GROUP BY clause * * GROUP BY items will be added to the targetlist (as resjunk columns) * if not already present, so the targetlist must be passed by reference. + * + * The order of the elements of the grouping clause does not affect + * the semantics of the query. However, the optimizer is not currently + * smart enough to reorder the grouping clause, so we try to do some + * primitive reordering here. */ List * transformGroupClause(ParseState *pstate, List *grouplist, List **targetlist, List *sortClause) { ! List *result = NIL; ! List *tle_list = NIL; ! ListCell *l; ! /* Preprocess the grouping clause, lookup TLEs */ ! foreach (l, grouplist) { TargetEntry *tle; ! Oid restype; ! tle = findTargetlistEntry(pstate, lfirst(l), targetlist, GROUP_CLAUSE); /* if tlist item is an UNKNOWN literal, change it to TEXT */ restype = exprType((Node *) tle->expr); *************** *** 1332,1356 **** restype = exprType((Node *) tle->expr); if (restype == UNKNOWNOID) - { tle->expr = (Expr *) coerce_type(pstate, (Node *) tle->expr, restype, TEXTOID, -1, COERCION_IMPLICIT, COERCE_IMPLICIT_CAST); - restype = TEXTOID; - } ! /* ! * If the GROUP BY clause matches the ORDER BY clause, we want to ! * adopt the ordering operators from the latter rather than using the ! * default ops. This allows "GROUP BY foo ORDER BY foo DESC" to be ! * done with only one sort step. Note we are assuming that any ! * user-supplied ordering operator will bring equal values together, ! * which is all that GROUP BY needs. ! */ ! if (sortItem && ! ((SortClause *) lfirst(sortItem))->tleSortGroupRef == ! tle->ressortgroupref) { ordering_op = ((SortClause *) lfirst(sortItem))->sortop; sortItem = lnext(sortItem); --- 1340,1369 ---- restype = exprType((Node *) tle->expr); if (restype == UNKNOWNOID) tle->expr = (Expr *) coerce_type(pstate, (Node *) tle->expr, restype, TEXTOID, -1, COERCION_IMPLICIT, COERCE_IMPLICIT_CAST); ! tle_list = lappend(tle_list, tle); ! } ! ! /* ! * Now iterate through the ORDER BY clause. If we find a grouping ! * element that matches the ORDER BY element, append the grouping ! * element to the result set immediately. Otherwise, stop ! * iterating. The effect of this is to look for a prefix of the ! * ORDER BY list in the grouping clauses, and to move that prefix ! * to the front of the GROUP BY. ! */ ! foreach (l, sortClause) ! { ! SortClause *sc = (SortClause *) lfirst(l); ! ListCell *prev = NULL; ! ListCell *tl; ! bool found = false; ! ! foreach (tl, tle_list) { TargetEntry *tle = (TargetEntry *) lfirst(tl); *************** *** 1352,1373 **** ((SortClause *) lfirst(sortItem))->tleSortGroupRef == tle->ressortgroupref) { ! ordering_op = ((SortClause *) lfirst(sortItem))->sortop; ! sortItem = lnext(sortItem); } - else - { - ordering_op = ordering_oper_opid(restype); - sortItem = NULL; /* disregard ORDER BY once match fails */ - } ! grpcl = makeNode(GroupClause); ! grpcl->tleSortGroupRef = assignSortGroupRef(tle, *targetlist); ! grpcl->sortop = ordering_op; ! glist = lappend(glist, grpcl); } ! return glist; } /* --- 1365,1420 ---- ((SortClause *) lfirst(sortItem))->tleSortGroupRef == tle->ressortgroupref) { ! TargetEntry *tle = (TargetEntry *) lfirst(tl); ! ! if (sc->tleSortGroupRef == tle->ressortgroupref) ! { ! GroupClause *gc; ! ! tle_list = list_delete_cell(tle_list, tl, prev); ! ! /* ! * This grouping clause matches one of the sorting ! * clause's ordering operator. ! */ ! gc = make_group_clause(tle, *targetlist, sc->sortop); ! result = lappend(result, gc); ! found = true; ! break; ! } ! ! prev = tl; } ! /* As soon as we've failed to match an ORDER BY element, stop */ ! if (!found) ! break; } ! /* ! * Now add any remaining elements of the GROUP BY list in the ! * order we received them. ! * ! * XXX: are there any additional criteria to consider when ! * ordering grouping clauses? ! */ ! foreach(l, tle_list) ! { ! TargetEntry *tle = (TargetEntry *) lfirst(l); ! GroupClause *gc; ! Oid sort_op; ! ! /* avoid making duplicate grouplist entries */ ! if (targetIsInSortList(tle, result)) ! continue; ! ! sort_op = ordering_oper_opid(exprType((Node *) tle->expr)); ! gc = make_group_clause(tle, *targetlist, sort_op); ! result = lappend(result, gc); ! } ! ! list_free(tle_list); ! return result; } /*
---------------------------(end of broadcast)--------------------------- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly