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