Re: [PATCH] Add support function for containment operators
От | Laurenz Albe |
---|---|
Тема | Re: [PATCH] Add support function for containment operators |
Дата | |
Msg-id | 3285c9f07e3f4af1ab910cc020454ba7e2307dfc.camel@cybertec.at обсуждение исходный текст |
Ответ на | Re: [PATCH] Add support function for containment operators (jian he <jian.universality@gmail.com>) |
Ответы |
Re: [PATCH] Add support function for containment operators
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Laurenz AlbeДата:
Сообщение: Re: [PATCH] Add support function for containment operators