bkietz commented on code in PR #36424:
URL: https://github.com/apache/arrow/pull/36424#discussion_r1254468172
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -461,6 +461,29 @@ TEST(Expression, IsSatisfiable) {
// fill_na)
EXPECT_TRUE(Bind(call("is_null", {never_true})).IsSatisfiable());
}
+
+ for (const auto& might_true : {
+ // N.B. this is "or_kleene"
+ or_(literal(false), field_ref("bool")),
+ or_(literal(null), field_ref("bool")),
+ call("or", {literal(false), field_ref("bool")}),
+ call("or", {literal(null), field_ref("bool")}),
+ }) {
+ ARROW_SCOPED_TRACE(might_true.ToString());
+ EXPECT_TRUE(Bind(might_true).IsSatisfiable());
+ }
+
+ for (const auto& never_true : {
+ // N.B. this is "or_kleene"
+ or_(literal(false), literal(null)),
+ call("or", {literal(false), literal(null)}),
Review Comment:
IsSatisfiable does not (currently) do any canonicalization or simplification
of the expression, so even things which could be trivially constant folded are
not caught. We usually only call IsSatisfiable after simplifying by a guarantee
or otherwise modifying an expression based on information available only to the
caller, so we shouldn't try simplifying in IsSatisfiable since that's probably
redundant and more expensive than we need.
@mapleFU could you add this to the doccomment for IsSatisfiable?
```diff
/// Return true if this expression could evaluate to true. Will return
true for any
- /// unbound, non-boolean, or unsimplified Expressions
+ /// unbound or non-boolean Expressions. IsSatisfiable does not
(currently) do any
+ /// canonicalization or simplification of the expression, so even
Expressions
+ /// which are be unsatisfiable may spuriously return `true` here. This
function is
+ /// intended for use in predicate pushdown where a filter expression is
simplified
+ /// by a guarantee, so it assumes that trying to simplify again would be
redundant.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
