Conversation
masonh22
left a comment
There was a problem hiding this comment.
This looks good, one small comment
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Outdated
Show resolved
Hide resolved
|
Thanks for your review @masonh22. I am thinking about |
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 Does that make sense? Is that what you were thinking about? |
|
If we pass a When looping through the expressions in the list we would check if But I don't think this is relevant because if you want |
|
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 |
comphead
left a comment
There was a problem hiding this comment.
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 ?
|
@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 I was thinking about adding support inside There is an issue for that: |
|
Given that some To avoid unidentified behaviors and bugs I propose to add slt tests to |
|
I personally think it would be better to put the tests in |
|
Given that the |
Which issue does this PR close?
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 LISTandNOT IN LISTsupport.Are there any user-facing changes?
No