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
v4-0002-Hash-a-ScalarArrayOpExpr-whose-array-is-fixed-for.patch
Description: Binary data
v4-0001-Refactor-hashed-ScalarArrayOpExpr-evaluation.patch
Description: Binary data
