Hi, Dmitry:
+ int lastExprLenght = 0;
Did you mean to name the variable lastExprLenghth ?
w.r.t. extracting to helper method, the second and third if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar.
It is up to you whether to create the helper method.
I am fine with the current formation.
Cheers
> On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> Hi,
> A few comments.
>
> + foreach(lc, (List *) expr)
> + {
> + Node * subExpr = (Node *) lfirst(lc);
> +
> + if (!IsA(subExpr, Const))
> + {
> + allConst = false;
> + break;
> + }
> + }
>
> It seems the above foreach loop (within foreach(temp, (List *) node)) can
> be preceded with a check that allConst is true. Otherwise the loop can be
> skipped.
Thanks for noticing. Now that I look at it closer I think it's the other
way around, the loop above checking constants for the first expression
is not really necessary.
> + if (currentExprIdx == pgss_merge_threshold - 1)
> + {
> + JumbleExpr(jstate, expr);
> +
> + /*
> + * A const expr is already found, so JumbleExpr must
> + * record it. Mark it as merged, it will be the
> first
> + * merged but still present in the statement query.
> + */
> + Assert(jstate->clocations_count > 0);
> + jstate->clocations[jstate->clocations_count -
> 1].merged = true;
> + currentExprIdx++;
> + }
>
> The above snippet occurs a few times. Maybe extract into a helper method.
Originally I was hesitant to extract it was because it's quite small
part of the code. But now I've realized that the part relevant to lists
is not really correct, which makes those bits even more different, so I
think it makes sense to leave it like that. What do you think?