Support simplify not for physical expr

[xudong963]

Part of the #18868

[xudong963]

Thanks for the review @martin-g

[alamb]

Thank you @xudong963 and @martin-g

I think prior to releasing this code we should make the following changes:

1. Avoid explicit recursion in not simplification (and instead use the tree node API)
2. Avoid the potential infinite loop

It would be nice to make them as part of this PR, but I also think it would be ok to do it as a follow on

Other things I think would be good to do but not strictly necessary:

1. Move the tests to the public interface (PhysicalExprSimplifier)
2. Avoid duplications of Operator::negate

[alamb]

I think you can use transform_data here instead: https://docs.rs/datafusion/latest/datafusion/common/tree_node/struct.Transformed.html#method.transform_data

So something like

// Apply NOT expression simplification firstlet rewritten =
simplify_not_expr(&node,self.schema)?.transform_data(|node| {
unwrap_cast::unwrap_cast_in_comparison(node,self.schema)})?;

That handles combining the transformed flag for you

[xudong963]

neat, done in be077db.

Need to catch up on the tree node APIs, haha

[alamb]

Since this function is not really run in isolation (it is always run as part of PhysicalExprSimplifier) I think it would be better if these tests were in datafusion/physical-expr/src/simplifier/mod.rs rather than here.

I don't think this change is required

[alamb]

This looks the same as Operator::negate: https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Operator.html#method.negate

I recommend using that code, or if there is a reason not to use Operator::negate add a comment explaining why it is not used

[alamb]

I think since simplify_not_expr is used in PhysicalExprSimplifier which is doing a walk up the plan (using f_up) there is no reason to also recurse explicitly in this rule.

You should be able to apply the rewrite rules only NOT(A OR B) --> NOT A AND NOT B rather than also changing the exprs

[alamb]

I am also somewhat concerned about this loop -- it seems like it may be trying to handle a non complete recursion and it could potentially lead to infinite recursion if a rewrite flip/flopped

If the loop is needed I suggest:

1. Put it in the higher level PhysicalExprSimplifier
2. add a counter that breaks after some number of iterations (e.g. 5 or something)

[xudong963]

I put it into PhysicalExprSimplifier with a counter to limit the loop

[alamb]

What is the reason for using to_string()?

I tried comparing the two directly and it seems to work

assert_eq!(&result.data,&inner_expr);

[xudong963]

yeah, I didn't notice the expr is comparable

[alamb]

It might also make these queries easier to read if you made a helper like

fnassert_transformed(expr,expected_expr);

and

fnassert_not_transformed(expr,expected_expr);

[alamb]

You could potentially make this a function to make the tests easier to read - something like

let literal = as_literal(&result);

And handle the panic internally in as_literal

That way the mechics of the test wouldn't obscure the test logic so much

I think you could do something similar with BinaryExpr

[alamb]

👍

[xudong963]

Thank you @xudong963 and @martin-g

I think prior to releasing this code we should make the following changes:

1. Avoid explicit recursion in not simplification (and instead use the tree node API)
2. Avoid the potential infinite loop

It would be nice to make them as part of this PR, but I also think it would be ok to do it as a follow on

Other things I think would be good to do but not strictly necessary:

1. Move the tests to the public interface (PhysicalExprSimplifier)
2. Avoid duplications of Operator::negate

Thanks @alamb. I addressed all suggestions in the latest commit 27c4ef1

[alamb]

Looks great @xudong963 -- thank you