Re: Hash a ScalarArrayOpExpr whose array is fixed for one execution

Hi David,

Thanks for the review. Attached is v4.

> Thanks for the explanations. They were useful. All of them should be
> turned into comments in the code.

Done.

> A comment saying why we cannot unify the Const and non-Const case in
> execExprInterp.c would be useful. One could think that it might be a
> good idea to now always go the non-Const route for code simplicity /
> consistency.

Done, above saop_hash_eval_array(). The non-Const route copies the
array, and a Const array needs no copy, because it is part of the plan.

> Doesn't saop_array_may_vary_walker() misses T_CaseWhen? T_CaseExpr's
> args is a list of CaseWhen nodes. When recursing it doesn't match
> against T_CaseWhen and returns true while CaseWhen doesn't vary. I think
> that would only occur in this form:
>
> x = ANY(CASE WHEN $2 THEN $1 ELSE '{...}' END

This form is hashed. The walker never gets a CaseWhen node, so
T_CaseWhen is not needed. I went through expression_tree_walker() and
ran a test to confirm this.

> - Use castNode() instead of C casts

Done.

> - Use a wrapper around saop_array_may_vary_walker(), e.g.
> saop_array_may_vary()

Done.

> Beyond that, I would make the commit message less technical. This level
> of detail should be code comments. The commit message should high-level
> outline the change and its limitations. You could put there e.g. where
> the optimization doesn't apply today and where it still doesn't apply
> wit the commit's changes (soft errors, standalone expressions).

Done. The commit message of 0002 now gives a high-level outline, and
says where the optimization still does not apply.

Any further feedback or guidance would be much appreciated.

Regards,
Linden