peter-toth commented on code in PR #11265:
URL: https://github.com/apache/datafusion/pull/11265#discussion_r1667004909
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -1838,4 +1851,53 @@ mod test {
Ok(())
}
+
+ #[test]
+ fn test_volatile() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let extracted_child = col("a") + col("b");
+ let rand =
Expr::ScalarFunction(ScalarFunction::new_udf(math::random(), vec![]));
+ let not_extracted_volatile = extracted_child + rand;
+ let plan = LogicalPlanBuilder::from(table_scan.clone())
+ .project(vec![
+ not_extracted_volatile.clone().alias("c1"),
+ not_extracted_volatile.alias("c2"),
+ ])?
+ .build()?;
+
+ let expected = "Projection: __common_expr_1 + random() AS c1,
__common_expr_1 + random() AS c2\
+ \n Projection: test.a + test.b AS __common_expr_1, test.a, test.b,
test.c\
+ \n TableScan: test";
+
+ assert_optimized_plan_eq(expected, plan, None);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_volatile_short_circuits() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let rand =
Expr::ScalarFunction(ScalarFunction::new_udf(math::random(), vec![]));
+ let not_extracted_volatile_short_circuit_2 =
+ rand.clone().eq(lit(0)).or(col("b").eq(lit(0)));
+ let not_extracted_volatile_short_circuit_1 =
+ col("a").eq(lit(0)).or(rand.eq(lit(0)));
+ let plan = LogicalPlanBuilder::from(table_scan.clone())
+ .project(vec![
+ not_extracted_volatile_short_circuit_1.clone().alias("c1"),
+ not_extracted_volatile_short_circuit_1.alias("c2"),
Review Comment:
This question is similar to
https://github.com/apache/datafusion/pull/11197#discussion_r1662618797. We can
extract the surely evaluated expressions (1st legs) from those short circuiting
`Or`, `And` and `CaseWhen` expressions, but we are not there yet with this PR.
My 3rd PR in the https://github.com/apache/datafusion/issues/11194 epic will
implement that logic, but I want to add the improvements gradually as it will
require to recurse into only certain children of a parent and that's not
straightforward with the current treenode APIs (i.e. we need to stop recursion
at the parent with a `Jump` and start a new recursion only on the interresting
children).
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -1838,4 +1851,53 @@ mod test {
Ok(())
}
+
+ #[test]
+ fn test_volatile() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let extracted_child = col("a") + col("b");
+ let rand =
Expr::ScalarFunction(ScalarFunction::new_udf(math::random(), vec![]));
+ let not_extracted_volatile = extracted_child + rand;
+ let plan = LogicalPlanBuilder::from(table_scan.clone())
+ .project(vec![
+ not_extracted_volatile.clone().alias("c1"),
+ not_extracted_volatile.alias("c2"),
+ ])?
+ .build()?;
+
+ let expected = "Projection: __common_expr_1 + random() AS c1,
__common_expr_1 + random() AS c2\
+ \n Projection: test.a + test.b AS __common_expr_1, test.a, test.b,
test.c\
+ \n TableScan: test";
+
+ assert_optimized_plan_eq(expected, plan, None);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_volatile_short_circuits() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let rand =
Expr::ScalarFunction(ScalarFunction::new_udf(math::random(), vec![]));
+ let not_extracted_volatile_short_circuit_2 =
+ rand.clone().eq(lit(0)).or(col("b").eq(lit(0)));
+ let not_extracted_volatile_short_circuit_1 =
+ col("a").eq(lit(0)).or(rand.eq(lit(0)));
+ let plan = LogicalPlanBuilder::from(table_scan.clone())
+ .project(vec![
+ not_extracted_volatile_short_circuit_1.clone().alias("c1"),
+ not_extracted_volatile_short_circuit_1.alias("c2"),
Review Comment:
This question is similar to
https://github.com/apache/datafusion/pull/11197#discussion_r1662618797. We can
extract the surely evaluated expressions (1st legs) from those short circuiting
`Or`, `And` and `CaseWhen` expressions, but we are not there yet with this PR.
My 3rd PR in the https://github.com/apache/datafusion/issues/11194 epic will
implement that logic, but I want to add the improvements gradually as it will
require to recurse into only certain children of a parent and that's not
straightforward with the current treenode APIs (i.e. we need to stop recursion
at the parent with a `Jump` and start a new recursion only on the interresting
children).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
