On Sun, Jan 7, 2024 at 4:59 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I don't think this is going in quite the right direction. We have > many serious problems with grouping sets (latest one today at [1]), > and I don't believe that hacking around EquivalenceClasses is going > to fix them all. > > I think that what we really need to do is invent a new kind of RTE > representing the output of the grouping step, with columns that > are the Vars or expressions being grouped on. Then we would make > the parser actually replace subexpressions in the tlist with Vars > referencing this new RTE (that is, change check_ungrouped_columns > into something that modifies the expression tree into something that > contains no Vars that aren't grouping-RTE Vars). In this way the > output of the parser directly expresses the semantic requirement that > certain subexpressions be gotten from the grouping output rather than > computed some other way. > > The trick is to do this without losing optimization capability. > We could have the planner replace these Vars with the underlying > Vars in cases where it's safe to do so (perhaps after adding a > nullingrel bit that references the grouping RTE). If a grouping > column is an expression, we might be able to replace the reference > Vars with PHVs as you've done here ... but I think we need the > parser infrastructure fixed first. Sorry it takes me some time to get back to this thread. I think you're right. To fix the cases where there are subqueries in the grouping sets, as in Geoff's example, it seems that we'll have to fix the parser infrastructure by inventing a new RTE for the grouping step and replacing the subquery expressions with Vars referencing this new RTE, so that there is only one instance of the subquery in the parser output. I have experimented with this approach, and here is the outcome. The patch fixes Geoff's query, but it's still somewhat messy as I'm not experienced enough in the parser code. And the patch has not yet implemented the nullingrel bit manipulation trick. Before proceeding further with this patch, I'd like to know if it is going in the right direction. Thanks Richard
v3-0001-Introduce-a-RTE-for-the-grouping-step.patch
Description: Binary data
