> That said, I think it might be better to continue this small
> optimization with NULL for constant arrays separately in another thread.
> It's cleaner to split this work into smaller, focused changes rather
> than mixing everything into single patch
Given that there's now a new thread about this, let's either remove it
from this patch and make sure we rebase correctly once the other patch
is merged, or use the latest version from this patch (ARR_HASNULL()
instead of memchr()).
> If anything is still unclear in the code or insufficiently documented,
> or if you have other suggestions, please do not hesitate to point them out.
- Currently we bail for unique Vars, via the following line.
var_eq_const() returns 1.0 / vardata->rel->tuples in that case. How
about we improve this case as well and, either, return
number_of_in_elements / vardata->rel->tuples, or fill the hash map and
then return number_of_unique_in_elements / vardata->rel->tuples. In
master this code is currently O(n). We could make it O(1) and keep the
today's semantics (count duplicates), or keep it O(n) but improve it by
not counting duplicates.
if (vardata->isunique && vardata->rel && vardata->rel->tuples >= 1.0)
return -1.0;
- How about factoring out the shared code from accum_scalararray_prob()
which is also used in the original estimation implementation? That would
get rid of some code duplication. Not sure how nice the code will get
though.
- The comment starting with the following line seems to have unintended
line breaks in various places (between the enumeration items):
Build arrays describing ARRAY[] elements:
- No need to use palloc0_array() for elem_values as all elements are
initialized anyways.
- Use lfirst_node() and castNode() instead of C type casts.
- Add a comment to the following line where you bail because an element
is NULL for the NOT IN case.
if (!useOr && elem_nulls[i])
- You can use foreach_current_index(). Then you don't need to declare
and update your own loop counter.
The rest looks good to me.
--
David Geier
