Re: Wrong results with grouping sets

On Sun, Jan 7, 2024 at 4:59 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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