Richard Guo <guofengli...@gmail.com> writes:
> Thank you for confirming. Here is an updated patch with some tweaks to
> the comments and commit message. I've parked this patch in the July
> commitfest.
I took a brief look at this. I think the basic idea is sound,
but I have a couple of nits:
* It's not entirely obvious that the checks preceding these additions
are sufficient to ensure that the clauses are OpExprs. They probably
are, since the clauses are marked hashable or mergeable, but that test
is mighty far away. More to the point, if they ever weren't OpExprs
the result would likely be to pass a bogus OID to get_commutator and
thus silently fail, allowing the problem to go undetected for a long
time. I'd suggest using castNode() rather than a hard-wired
assumption that the clause is an OpExpr.
* Do we really need to construct a whole new set of bogus operators
and opclasses to test this? As you noted, the regression tests
already set up an incomplete opclass for other purposes. Why can't
we extend that test, to reduce the amount of cycles wasted forevermore
on this rather trivial point?
(I'm actually wondering whether we really need to memorialize this
with a regression test case at all. But I'd settle for minimizing
the amount of test cycles added.)
regards, tom lane
