On Fri, Mar 8, 2024 at 7:43 AM David Rowley <dgrowle...@gmail.com> wrote:
> On Fri, 8 Mar 2024 at 00:54, Ashutosh Bapat > <ashutosh.bapat....@gmail.com> wrote: > > > > On Thu, Mar 7, 2024 at 4:39 PM David Rowley <dgrowle...@gmail.com> > wrote: > >> I think the fix should go in appendOrderByClause(). It's at that > >> point we look for the EquivalenceMember for the relation and can > >> easily discover if the em_expr is a Const. I think we can safely just > >> skip doing any ORDER BY <const> stuff and not worry about if the > >> literal format of the const will appear as a reference to an ordinal > >> column position in the ORDER BY clause. > > > > deparseSortGroupClause() calls deparseConst() with showtype = 1. > appendOrderByClause() may want to do something similar for consistency. Or > remove it from deparseSortGroupClause() as well? > > The fix could also be to use deparseConst() in appendOrderByClause() > and have that handle Const EquivalenceMember instead. I'd rather just > skip them. To me, that seems less risky than ensuring deparseConst() > handles all Const types correctly. > > Also, as far as adjusting GROUP BY to eliminate Consts, I don't think > that's material for a bug fix. If we want to consider doing that, > that's for master only. > If appendOrderByClause() would have been using deparseConst() since the beginning this bug would not be there. Instead of maintaining two different ways of deparsing ORDER BY clause, we could maintain just one. I think we should unify those. If we should do it in only master be it so. I am fine to leave back branches with two methods. > > >> I wonder if we need a test... > > > > Yes. > > I've added two of those in the attached. > > Thanks. They look fine to me. -- Best Wishes, Ashutosh Bapat
