Thanks for the review!
It was the first version for discussion. Of course, refactoring and
polishing cycles will be needed, even so we can discuss the general idea
earlier.
On 10/2/2024 12:00, jian he wrote:
> On Thu, Feb 8, 2024 at 1:34 PM Andrei Lepikhov
> 1235 | PredicatesData *pd;
Thanks
> + if (!predicate_implied_by(index->indpred, list_make1(rinfo1), true))
> + elog(ERROR, "Logical mistake in OR <-> ANY transformation code");
> the error message seems not clear?
Yeah, have rewritten
> static List *
> build_paths_for_SAOP(PlannerInfo *root, RelOptInfo *rel, RestrictInfo *rinfo,
> List *other_clauses)
> I am not sure what's `other_clauses`, and `rinfo` refers to? adding
> some comments would be great.
>
> struct PredicatesData needs some comments, I think.
Added, not so much though
>
> +bool
> +saop_covered_by_predicates(ScalarArrayOpExpr *saop, List *predicate_lists)
> +{
> + ListCell *lc;
> + PredIterInfoData clause_info;
> + bool result = false;
> + bool isConstArray;
> +
> + Assert(IsA(saop, ScalarArrayOpExpr));
> is this Assert necessary?
Not sure. Moved it into another routine.
>
> For the function build_paths_for_SAOP, I think I understand the first
> part of the code.
> But I am not 100% sure of the second part of the `foreach(lc,
> predicate_lists)` code.
> more comments in `foreach(lc, predicate_lists)` would be helpful.
Done
>
> do you need to add `PredicatesData` to src/tools/pgindent/typedefs.list?
Done
>
> I also did some minor refactoring of generate_saop_pathlist.
Partially agree
>
> instead of let it go to `foreach (lc, entries)`,
> we can reject the Const array at `foreach(lc, expr->args)`
Yeah, I just think we can go further and transform two const arrays into
a new one if we have the same clause and operator. In that case, we
should allow it to pass through this cycle down to the classification stage.
>
> also `foreach(lc, expr->args)` do we need to reject cases like
> `contain_subplans((Node *) nconst_expr)`?
> maybe let the nconst_expr be a Var node would be far more easier.
It's contradictory. On the one hand, we simplify the comparison
procedure and can avoid expr jumbling at all. On the other hand - we
restrict the feature. IMO, it would be better to unite such clauses
complex_clause1 IN (..) OR complex_clause1 IN (..)
into
complex_clause1 IN (.., ..)
and don't do duplicated work computing complex clauses.
In the attachment - the next version of the second patch.
--
regards,
Andrei Lepikhov
Postgres Professional