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