Enable inlist support for preimage

[sdf-jkl]

Which issue does this PR close?

• ClosesInList support for pre-image udf #20050

Rationale for this change

Check issue

What changes are included in this PR?

Match arm to support preimage for InList expressions in expr_simplifier.rs

Are these changes tested?

Yes, added two tests for IN LIST and NOT IN LIST support.

Are there any user-facing changes?

No

[masonh22]

This looks good, one small comment

[sdf-jkl]

Thanks for your review @masonh22.

I am thinking about NULL support inside InList expressions for this simplification. I don't think InList supports distinct behavior like is distinct from or is not distinct from, so we should probably avoid it.

[masonh22]

I am thinking about NULL support inside InList expressions for this simplification. I don't think InList supports distinct behavior like is distinct from or is not distinct from, so we should probably avoid it.

Hmmm, I see that you are already checking whether the list contains null and not applying the rule in that case. I'm not really sure what the semantics are for null values in InList. I see that ShortenInListSimplifier, which rewrites InList expressions as a == x || a == y ... (or a != x && a != y ... if it's negated), doesn't check whether any of the values are null. So from that perspective, I think it would be fine to pass a null literal value into get_preimage().

Does that make sense? Is that what you were thinking about?

[sdf-jkl]

If we pass a Null literal in get_preimage as is, it will not return PreimageResult::Range and the whole simplification will be aborted.
I was thinking about checking if any(is_null) and storing it. When true, we would add expr.is_null() or expr.is_not_null() to the resulting expression. It will be based on the negate boolean(false => is_null(), true => is_not_null()).

When looping through the expressions in the list we would check if expr.is_null(), and if it is continue, else get_preimage(expr).

But I don't think this is relevant because if you want distinct behavior, you should chain is_(not_)distinct_from expressions instead of using InList.

[masonh22]

Oh right I see what you mean. I think you could skip over nulls while looping over the expressions but I don't think you need to add expr.is_null() to the resulting expression. Null values in the InList array should have no impact on what it evaluates to if it doesn't use the is distinct from semantics.

[comphead]

Thanks @sdf-jkl please help me to understand how inlist preimage is different from datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs

And if it is, maybe the file above would be the better place for InList ?

[sdf-jkl]

@comphead The issue with existing inlist_simplifier.rs is this guard:

}) = expr
&& !list.is_empty()
&& (// For lists with only 1 value we allow more complex expressions to be simplified// e.g SUBSTR(c1, 2, 3) IN ('1') -> SUBSTR(c1, 2, 3) = '1'// for more than one we avoid repeating this potentially expensive// expressions
list.len() == 1
|| list.len() <= THRESHOLD_INLINE_INLIST
&& expr.try_as_col().is_some()// <--- this point here)

When list.len() is greater than 1, we only perform this simplification if lhs is a Column expression. Expressions where the column wrapped in a function are not handled.

I was thinking about adding support inside inlist_simplifier.rs, but there is a precedent for handling InList support inside expr_simplifier.rs for unwrap_cast. If move unwrap_cast and preimageInList support here, we'd need another guard to check which function is being passed. Currently, the only way is by matching function name, which isn't neat.

There is an issue for that:

• Avoid check function type by matching names in the optimizer #18643

[alamb]

Thanks @sdf-jkl and @comphead

[sdf-jkl]

Given that some preimage implementations got merged earlier than this PR, date_part and floor will have preimage InList support once this is merged.

To avoid unidentified behaviors and bugs I propose to add slt tests to date_part.slt and floor_preimage.slt here.

[alamb]

I personally think it would be better to put the tests in date_part.slt and floor.slt -- not have a seprate _preimage variant -- that will make the tests easier to find in the future

[sdf-jkl]

Given that the floor slt tests were created in floor_preimage in #20099, I also added the InList tests there.

[alamb]

Thanks @sdf-jkl@comphead and @masonh22