James Coleman <jtc331@gmail.com> writes:
> [ saop_is_not_null-v10.patch ]
I went through this again, and this time (after some more rewriting
of the comments) I satisfied myself that the logic is correct.
Hence, pushed.
I stripped down the regression test cases somewhat. Those were
good for development, but they were about doubling the runtime of
test_predtest.sql, which seemed excessive for testing one feature.
I also tweaked it to recognize the case where we can prove the
array, rather than the scalar, to be null. I'm not sure how useful
that is in practice, but it seemed weird that we'd exploit that
only if we can also prove the scalar to be null.
> I've implemented this fix as suggested. The added argument I've
> initially called "allow_false"; I don't love the name, but it is
> directly what it does. I'd appreciate other suggestions (if you prefer
> it change).
I was tempted to rename it to "weak", but decided that that might be
overloading that term too much in this module. Left it as-is.
> Ideally I think this case would be handled elsewhere in the optimizer
> by directly replacing the saop and a constant NULL array with NULL,
> but this isn't the patch to do that, and at any rate I'd have to do a
> bit more investigation to understand how to do such (if it's easily
> possible).
Take a look at the ScalarArrayOp case in eval_const_expressions.
Right now it's only smart about the all-inputs-constant case.
I'm not really convinced it's worth spending cycles on the constant-
null-array case, but that'd be where to do it if we want to do it
in a general way. (I think that what I added to clause_is_strict_for
is far cheaper, because while it's the same test, it's in a place
that we won't hit during most queries.)
> I also updated the tests with a new helper function "opaque_array" to
> make it easy to test cases where the planner can't inspect the array.
Yeah, that's a win. I used that in most of the tests that I left in
place, but I kept a couple with long arrays just so we'd have code
coverage of the parts of clause_is_strict_for that need to detect
array size.
regards, tom lane
