consider volatile function in simply_expression #13128

Lordworms · 2024-10-26T20:55:03Z

Which issue does this PR close?

Closes #13060

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jonathanc-n

this lgtm!

peter-toth · 2024-10-27T08:57:16Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

            // Eliminate common factors in conjunctions e.g
            // (A AND B) OR (A AND C) -> A AND (B OR C)
            Expr::BinaryExpr(BinaryExpr {
                left,
                op: Or,
                right,
-            }) if has_common_conjunction(&left, &right) => {
+            }) if has_common_conjunction(&left, &right)? => {


This doesn't seem correct. It is ok to just check the presence of common non-volatile expressions in has_common_conjunction() to trigger simplification, but you need to deal with volatile expressions during simplification too and don't extract them as common.

E.g. can you please add a test like column1 = 2 AND random() = 0 OR column1 = 2 AND random() = 0?

I checked postgresql and duckdb, they seems to merge the volatile expressions

That's weird and seems incorrect to me. BTW, this is what Spark does:

scala> sql("select * from t where column1 = 2 AND rand() = 0 OR column1 = 2 AND rand() = 0").explain(true); == Parsed Logical Plan == 'Project [*] +- 'Filter ((('column1 = 2) AND ('rand() = 0)) OR (('column1 = 2) AND ('rand() = 0))) +- 'UnresolvedRelation [t], [], false == Analyzed Logical Plan == column1: int, column2: int Project [column1#0, column2#1] +- Filter (((column1#0 = 2) AND (rand(-1409433140814249342) = cast(0 as double))) OR ((column1#0 = 2) AND (rand(2383542950853957003) = cast(0 as double)))) +- SubqueryAlias spark_catalog.default.t +- HiveTableRelation [`spark_catalog`.`default`.`t`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, Data Cols: [column1#0, column2#1], Partition Cols: []] == Optimized Logical Plan == Filter (isnotnull(column1#0) AND ((column1#0 = 2) AND ((rand(-1409433140814249342) = 0.0) OR (rand(2383542950853957003) = 0.0)))) +- HiveTableRelation [`spark_catalog`.`default`.`t`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, Data Cols: [column1#0, column2#1], Partition Cols: []] == Physical Plan == *(1) Filter ((isnotnull(column1#0) AND (column1#0 = 2)) AND ((rand(-1409433140814249342) = 0.0) OR (rand(2383542950853957003) = 0.0))) +- Scan hive spark_catalog.default.t [column1#0, column2#1], HiveTableRelation [`spark_catalog`.`default`.`t`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, Data Cols: [column1#0, column2#1], Partition Cols: []]

datafusion/optimizer/src/simplify_expressions/utils.rs

peter-toth · 2024-10-28T08:39:24Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

    let lhs: HashSet<&Expr> = iter_conjunction(lhs).collect();
-    iter_conjunction(rhs).any(|e| lhs.contains(&e))
+    iter_conjunction(rhs).try_fold(false, |acc, e| {


This still looks a bit ugly. I mean, we just look for the first non-volatile expression and don't want to iterate over all expressions.

Actually, I think I made a mistake when I added is_volatile() with return type Result<bool>:

datafusion/datafusion/expr/src/expr.rs

Lines 1581 to 1583 in e22d231

pub fn is_volatile(&self) -> Result<bool> {

self.exists(|expr| Ok(expr.is_volatile_node()))

}

As the closure we provide to exists() can't return any error, it would be safe to unwrap the result and return only bool.

@alamb, do you think we can fix Expr::is_volatile(), it would be an API breaking change though, to make it simpler and so we can keep using any() here?

I have refactored it, Thanks for your review

Looks good to me, thanks for the fix.

Maybe we can consider changing the return type of Expr::is_volatile() in a follow-up PR.

Maybe we can consider changing the return type of Expr::is_volatile() in a follow-up PR.

I think this PR would be much cleaner if we fixed the signature of that first. But I guess the important thing is that we do fix it before it spreads keeps spreading.

I changed the signature in #13206 which caused a logical conflict in this PR (failed to compile) and then I updated this PR to use the new signature

As @eejbyfeldt predicated, the PR became much cleaner

eejbyfeldt · 2024-10-30T13:18:40Z

datafusion/sqllogictest/test_files/explain.slt

+
+
+
+query TT


Any reason for not adding these as unit tests in the expr_simplifier module instead?

They feel quite output place in file called explain.slt.

eejbyfeldt · 2024-10-30T13:20:14Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

    let lhs: HashSet<&Expr> = iter_conjunction(lhs).collect();
-    iter_conjunction(rhs).any(|e| lhs.contains(&e))
+    iter_conjunction(rhs).try_fold(false, |acc, e| {


Maybe we can consider changing the return type of Expr::is_volatile() in a follow-up PR.

I think this PR would be much cleaner if we fixed the signature of that first. But I guess the important thing is that we do fix it before it spreads keeps spreading.

alamb

Thanks @Lordworms @findepi and @peter-toth

I agree with @eejbyfeldt in https://github.com/apache/datafusion/pull/13128/files#r1822606751 that we should add some unit tests in datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs in addition to the explain .slt test

I also have a proposal that I think would simplify this PR: #13206

alamb · 2024-10-31T19:04:13Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+fn has_common_conjunction(lhs: &Expr, rhs: &Expr) -> Result<bool, DataFusionError> {
+    let lhs_set: HashSet<&Expr> = iter_conjunction(lhs).collect();
+    for e in iter_conjunction(rhs) {
+        if lhs_set.contains(&e) && !e.is_volatile()? {


I looked a little more at Expr::is_volatile and I actually think it can't return any errors. If we changed the signature I think this PR would be significantly simpler

alamb

Thanks @Lordworms

alamb · 2024-10-31T23:04:43Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                .eq(lit(2))
+                .or(col("column1").eq(lit(2)).and(rand.clone().eq(lit(0))));
+
+            assert_eq!(simplify(expr), col("column1").eq(lit(2)));


It would seem to me this should still be the same thing -- namely that we shouldn't remove the rand() = 0 condition 🤔

However, this PR still seems like it is an improvement over what is on main so we could potentially fix this in a follow on PR

Could be something wrong here, let me double check

I think here should remove the rand() = 0 since we must fulfill the column1 = 2 first? according to the description in the issue? Don't know if I got it wrong....

Also Happy Halloween!

alamb · 2024-11-01T16:56:39Z

I merged up from main to resolve a logical conflict with this PR

alamb · 2024-11-01T18:12:27Z

Thanks again @Lordworms and @eejbyfeldt and @peter-toth and @jonathanc-n

consider volatile function in simply_expression

b9fa3ce

github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 26, 2024

jonathanc-n approved these changes Oct 27, 2024

View reviewed changes

peter-toth reviewed Oct 27, 2024

View reviewed changes

datafusion/optimizer/src/simplify_expressions/utils.rs Outdated Show resolved Hide resolved

Lordworms added 2 commits October 27, 2024 16:16

refactor and fix bugs

579ee71

fix clippy

fb705be

peter-toth reviewed Oct 28, 2024

View reviewed changes

refactor

913dce8

peter-toth approved these changes Oct 29, 2024

View reviewed changes

eejbyfeldt reviewed Oct 30, 2024

View reviewed changes

alamb mentioned this pull request Oct 31, 2024

Minor: make Expr::volatile infallible #13206

Merged

alamb approved these changes Oct 31, 2024

View reviewed changes

refactor

87e7bc1

github-actions bot added the core Core DataFusion crate label Oct 31, 2024

Lordworms added 2 commits October 31, 2024 13:51

format

53f2b85

fix clippy

41c40d4

github-actions bot removed the core Core DataFusion crate label Oct 31, 2024

alamb reviewed Oct 31, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

alamb added 2 commits November 1, 2024 12:49

Merge remote-tracking branch 'apache/main' into issue_13060

6286301

Resolve logical conflict

eb546b3

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Nov 1, 2024

simplify more

ccd665a

alamb merged commit 6b76a35 into apache:main Nov 1, 2024
24 checks passed

theirix mentioned this pull request Nov 22, 2024

Filters on RANDOM() are applied incorrectly when pushdown_filters is enabled. #13268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consider volatile function in simply_expression #13128

consider volatile function in simply_expression #13128

Lordworms commented Oct 26, 2024

jonathanc-n left a comment

peter-toth Oct 27, 2024

Lordworms Oct 27, 2024

Lordworms Oct 27, 2024

peter-toth Oct 28, 2024

peter-toth Oct 28, 2024 •

edited

Loading

Lordworms Oct 28, 2024

peter-toth Oct 29, 2024 •

edited

Loading

eejbyfeldt Oct 30, 2024

alamb Nov 1, 2024

eejbyfeldt Oct 30, 2024

Lordworms Oct 30, 2024

eejbyfeldt Oct 30, 2024

alamb left a comment

alamb Oct 31, 2024

alamb left a comment

alamb Oct 31, 2024

alamb Oct 31, 2024

Lordworms Oct 31, 2024

Lordworms Oct 31, 2024

Lordworms Oct 31, 2024

alamb Nov 1, 2024

This comment was marked as outdated.

alamb commented Nov 1, 2024

alamb commented Nov 1, 2024 •

edited

Loading

	pub fn is_volatile(&self) -> Result<bool> {
	self.exists(\|expr\| Ok(expr.is_volatile_node()))
	}




		query TT

consider volatile function in simply_expression #13128

consider volatile function in simply_expression #13128

Conversation

Lordworms commented Oct 26, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jonathanc-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

alamb commented Nov 1, 2024

alamb commented Nov 1, 2024 • edited Loading

peter-toth Oct 28, 2024 •

edited

Loading

peter-toth Oct 29, 2024 •

edited

Loading

alamb commented Nov 1, 2024 •

edited

Loading