On Mon, Jan 14, 2019 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> James Coleman <jtc331@gmail.com> writes:
> > On Mon, Jan 14, 2019 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think these test cases don't actually prove much about the behavior
> >> of your patch. Wouldn't they be handled by the generic OR-conversion
> >> logic, since there's fewer than MAX_SAOP_ARRAY_SIZE items?
>
> > Which ones are you looking at in particular? The "inline" (non-cte)
> > happy and sad path cases have 102 total array elements (as does the
> > happy path cte case), and MAX_SAOP_ARRAY_SIZE is 100. The other two
> > tests are about the empty array case so much have 0 items (and were
> > the ones that showed different behavior between v3 and v4).
>
> I was looking at the empty-array ones. I don't see how that reaches
> your patch; we should not be doing predicate_implied_by_simple_clause
> on the ScalarArrayOp itself, because predicate_classify should've
> decided to treat it as an OR clause.
At one point I was getting all 'f' instead of all 't' for those, but
now I can't reproduce it going from either master to my patch or to my
patch without the element count checks, so I'm not sure how/when I was
getting that anymore.
Are those invariants we want to keep (and recognize as regression
tests)? If so, I can confirm that they aren't duplicative of the rest
of the file, and if not I can remove them.
James Coleman
