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 Tue, Jan 5, 2021 at 4:51 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > 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?
>
