Thanks a lot for v2 -- this is a really nice cleanup. Resolving the
CaseTestExpr through case_var and reusing grouping_check_operand() is much
tidier than the special-cased v1, it correctly handles the relabel/opfamily
mismatch that started the thread, and it addresses Yuhang's concern about
the WHEN condition not always being a bare OpExpr. Overall this looks good
to me.
On correctness I'm happy: I built it --with-icu and poked at the unsafe
directions -- the original citext relabel case, and a WHEN with an explicit
differing COLLATE -- and v2 keeps both above the grouping as it should.
CaseTestExpr is a leaf, and for the CASE path v2 checks strictly more than
before (it adds the opfamily check), so I don't think it can miss a conflict
the old code caught. No wrong-results case turned up. +1 from me on the
approach.
One minor thing I noticed while testing (a missed pushdown, not a
correctness problem): v2 can resolve CaseTestExprs that don't actually
belong to the CASE arg, and over a nondeterministic collation that shows up
as a spurious refusal to push a safe qual. For example:
CREATE COLLATION ci (provider=icu, locale='und-u-ks-level2',
deterministic=false);
CREATE TABLE cit (t text COLLATE ci);
INSERT INTO cit VALUES ('a'),('A'),('b');
-- pushed on master, but v2 keeps the Filter above the grouping
EXPLAIN (COSTS OFF)
SELECT * FROM (SELECT DISTINCT t FROM cit) d
WHERE (CASE t WHEN JSON_OBJECT('a': 'b' RETURNING text)
THEN 1 ELSE 0 END) = 1;
The WHEN value is a constant that doesn't reference t, so this is safe to
push; the plain-constant version, JSON_OBJECT(...)::text (instead of
RETURNING text), and the same query over a deterministic collation are all
still pushed on the v2 build, so it's specifically the stray CaseTestExpr
that blocks it. The culprit is that JSON_OBJECT(... RETURNING text) carries
its own CaseTestExpr for the RETURNING coercion (makeJsonConstructorExpr()),
standing for the constructor output rather than the CASE arg; walked as a
non-operand with case_var bound, it gets resolved to the (nondeterministic)
grouping column and reported as a conflict. It's the same shape you already
shield for the ArrayCoerceExpr elemexpr -- JSON constructors are just another
producer that isn't (parse_jsontable.c / parse_target.c emit some too).
Thanks again -- nice work on this.
On Fri, Aug 28, 2026 at 10:20 AM Richard Guo <[email protected]> wrote:
>
> On Tue, Aug 18, 2026 at 9:54 AM Tender Wang <[email protected]> wrote:
> > On master, the qual is pushed below the DISTINCT:
> >
> > ```text
> > Unique
> > -> Sort
> > Sort Key: cit.t
> > -> Seq Scan on cit
> > Filter: (CASE (t)::text WHEN 'A'::text THEN 1 ELSE 0 END = 1)
> > ```
> >
> > This is suspicious because DISTINCT compares `t` using citext
> > equality, under which `'a'` and `'A'` are equal, while the CASE
> > expression casts `t` to text and therefore distinguishes them.
>
> Thanks for the report. I looked into it and I think the root cause is
> that grouping_conflict_walker() treats the arg of a simple CASE as an
> operand of each WHEN comparison, but only applied the collation half
> of the direct-operand check, on the assumption that the WHEN operator
> is always the type-default "=" and thus matches the grouping eqop.
> That is not true once the arg is relabeled. In your case with "CASE
> t::text WHEN 'A'", the WHEN compares with texteq while the grouping
> uses citext_eq, so the qual should be rejected just as "t::text = 'A'"
> already is.
>
> I reviewed your patch. IIUC, it fixes this by adding an
> equality_ops_are_compatible() check per WHEN inside the CaseExpr
> branch. That works for the reported query, but I'd rather not go that
> way, for a few reasons.
>
> It duplicates the operand check that grouping_check_operand already
> implements, so the two would have to be kept in sync.
>
> It assumes each WHEN condition is a bare OpExpr (the Assert), which
> the parser does not guarantee. It also skips the
> op_is_safe_index_member() gate, which is what makes the opfamily test
> meaningful for the direct-operand form.
>
> More generally, the problem is that the CaseExpr branch re-implements
> how a direct operand is checked, and does so incompletely.
>
> I think it'd be better to avoid this duplication. So I'd like to take
> the approach used elsewhere in planner for the same placeholder: while
> walking the WHEN conditions, the walker binds a Var arg in the context
> and resolves each CaseTestExpr to it. The Var is then checked as each
> WHEN uses it. This is how eval_const_expressions() handles the
> CaseTestExpr nodes.
>
> Attached is the patch doing that.
>
> - Richard
--
Regards,
Ewan Young
