Re: [PATCH] Add support function for containment operators

On Fri, 2023-10-20 at 16:24 +0800, jian he wrote:
> [new patch]

Thanks, that patch works as expected and passes regression tests.

Some comments about the code:

> --- a/src/backend/utils/adt/rangetypes.c
> +++ b/src/backend/utils/adt/rangetypes.c
> @@ -558,7 +570,6 @@ elem_contained_by_range(PG_FUNCTION_ARGS)
> PG_RETURN_BOOL(range_contains_elem_internal(typcache, r, val));
> }
>
> -
> /* range, range -> bool functions */
>
> /* equality (internal version) */

Please don't change unrelated whitespace.

> +static Node *
> +find_simplified_clause(Const *rangeConst, Expr *otherExpr)
> +{
> + Form_pg_range pg_range;
> + HeapTuple tup;
> + Oid opclassOid;
> + RangeBound lower;
> + RangeBound upper;
> + bool empty;
> + Oid rng_collation;
> + TypeCacheEntry *elemTypcache;
> + Oid opfamily = InvalidOid;
> +
> + RangeType *range = DatumGetRangeTypeP(rangeConst->constvalue);
> + TypeCacheEntry *rangetypcache = lookup_type_cache(RangeTypeGetOid(range), TYPECACHE_RANGE_INFO);
> + {

This brace is unnecessary. Perhaps a leftover from a removed conditional statement.

> + /* this part is get the range's SUBTYPE_OPCLASS from pg_range catalog.
> + * Refer load_rangetype_info function last line.
> + * TypeCacheEntry->rngelemtype typcaheenetry either don't have opclass entry or with default opclass.
> + * Range's subtype opclass only in catalog table.
> + */

The comments in the patch need some more love.
Apart from the language, you should have a look at the style guide:

- single-line comments should start with lower case and have no period:

/* example of a single-line comment */

- Multi-line comments should start with /* on its own line and end with */ on its
own line. They should use whole sentences:

/*
* In case a comment spans several lines, it should look like
* this. Try not to exceed 80 characters.
*/

> + tup = SearchSysCache1(RANGETYPE, ObjectIdGetDatum(RangeTypeGetOid(range)));
> +
> + /* should not fail, since we already checked typtype ... */
> + if (!HeapTupleIsValid(tup))
> + elog(ERROR, "cache lookup failed for range type %u", RangeTypeGetOid(range));

If this is a "can't happen" case, it should be an Assert.

> +
> + pg_range = (Form_pg_range) GETSTRUCT(tup);
> +
> + opclassOid = pg_range->rngsubopc;
> +
> + ReleaseSysCache(tup);
> +
> + /* get opclass properties and look up the comparison function */
> + opfamily = get_opclass_family(opclassOid);
> + }
> +
> + range_deserialize(rangetypcache, range, &lower, &upper, &empty);
> + rng_collation = rangetypcache->rng_collation;
> +
> + if (empty)
> + {
> + /* If the range is empty, then there can be no matches. */
> + return makeBoolConst(false, false);
> + }
> + else if (lower.infinite && upper.infinite)
> + {
> + /* The range has no bounds, so matches everything. */
> + return makeBoolConst(true, false);
> + }
> + else
> + {

Many of the variables declared at the beginning of the function are only used in
this branch. You should declare them here.

> +static Node *
> +match_support_request(Node *rawreq)
> +{
> + if (IsA(rawreq, SupportRequestSimplify))
> + {

To keep the indentation shallow, the preferred style is:

if (/* case we don't handle */)
return NULL;
/* proceed without indentation */

> + SupportRequestSimplify *req = (SupportRequestSimplify *) rawreq;
> + FuncExpr *fexpr = req->fcall;
> + Node *leftop;
> + Node *rightop;
> + Const *rangeConst;
> + Expr *otherExpr;
> +
> + Assert(list_length(fexpr->args) == 2);
> +
> + leftop = linitial(fexpr->args);
> + rightop = lsecond(fexpr->args);
> +
> + switch (fexpr->funcid)
> + {
> + case F_ELEM_CONTAINED_BY_RANGE:
> + if (!IsA(rightop, Const) || ((Const *) rightop)->constisnull)
> + return NULL;
> +
> + rangeConst = (Const *) rightop;
> + otherExpr = (Expr *) leftop;
> + break;
> +
> + case F_RANGE_CONTAINS_ELEM:
> + if (!IsA(leftop, Const) || ((Const *) leftop)->constisnull)
> + return NULL;
> +
> + rangeConst = (Const *) leftop;
> + otherExpr = (Expr *) rightop;
> + break;
> +
> + default:
> + return NULL;
> + }
> +
> + return find_simplified_clause(rangeConst, otherExpr);
> + }
> + return NULL;
> +}
> \ No newline at end of file

You are calling this funtion from both "elem_contained_by_range_support" and
"range_contains_elem_support", only to branch based on the function type.
I think the code would be simpler if you did away with "match_support_request" at all.

I adjusted your patch according to my comments; what do you think?

I also went over the regression tests. I did away with the comparison function, instead
I used examples that don't return too many rows. I cut down on the test cases a little
bit. I added a test that uses the "text_pattern_ops" operator class.

Yours,
Laurenz Albe