Generic constant expression evaluation #1153

alamb · 2021-10-20T19:17:43Z

Which issue does this PR close?

Closes #1070.

Rationale for this change

This basically lets DataFusion rewrite expressions like the following at plan time rather than once per row

5 < 10 --> true
substr('foobar', 1 2) --> 'oo'

This is useful when predicates are created by some other system (e.g. a GUI) or other optimization passes

It works for all expression types that can be evaluated, including simple (e.g. +), complex (e.g. CASE) , and user defned functions.

Check out some examples within the datafusion-cli:

> explain select 1 + 2;
+---------------+--------------------------------------+
| plan_type     | plan                                 |
+---------------+--------------------------------------+
| logical_plan  | Projection: Int64(3)                 |
|               |   EmptyRelation                      |
| physical_plan | ProjectionExec: expr=[3 as Int64(3)] |
|               |   EmptyExec: produce_one_row=true    |
|               |                                      |
+---------------+--------------------------------------+
2 rows in set. Query took 0.005 seconds.
> explain select case when true then 'Monday' else 'Friday' end;
+---------------+-------------------------------------------------+
| plan_type     | plan                                            |
+---------------+-------------------------------------------------+
| logical_plan  | Projection: Utf8("Monday")                      |
|               |   EmptyRelation                                 |
| physical_plan | ProjectionExec: expr=[Monday as Utf8("Monday")] |
|               |   EmptyExec: produce_one_row=true               |
|               |                                                 |
+---------------+-------------------------------------------------+
2 rows in set. Query took 0.002 seconds.

Note there is also prior work from @pjmore from #1128 which is partially included in this pr (see #1128 (comment))

What changes are included in this PR?

ConstEvalator expression rewriter
Integrate ConstEvaluator into ConstantFolding optimizer pass
Additional test coverage

Follow on work

Are there any user-facing changes?

New ConstEvalator in optimizer utilities
More expressions are folded

alamb · 2021-10-21T11:12:58Z

datafusion/src/optimizer/constant_folding.rs

                    .query_execution_start_time
                    .timestamp_nanos(),
            ))),
-            Expr::ScalarFunction {


The rewrite of to_timestamp no longer needs to be special cased -- it now handled, along with other functions, by ConstEvaluator

datafusion/src/optimizer/constant_folding.rs

alamb · 2021-10-21T11:16:57Z

datafusion/src/optimizer/constant_folding.rs

+impl<'a> ExprRewriter for Simplifier<'a> {
    /// rewrite the expression simplifying any constant expressions
    fn mutate(&mut self, expr: Expr) -> Result<Expr> {
        let new_expr = match expr {


The code to handle true != true type expressions should also be handled by ConstEvaluator but it turns out there is a missing support for such degenerate expressions in the Expr evaluation code. I will fix this but plan to do so as a follow on PR to keep this PR reasonable to review

alamb · 2021-10-21T11:20:20Z

datafusion/src/optimizer/constant_folding.rs

            .build()
            .unwrap();

-        let expected = "Projection: totimestamp(Utf8(\"I\'M NOT A TIMESTAMP\"))\


Previously such plans would error at runtime, now they error at plan time which I think is a reasonable change in behavior (earlier errors are better errors?)

It might have unfortunate edge cases where a plan like SELECT to_timestamp('FOOO') from table_with_no_rows previously would error but now it will error.

However, I don't think this is an important difference

alamb · 2021-10-21T11:21:21Z

datafusion/src/optimizer/constant_folding.rs

-    }
-
-    #[test]
-    fn to_timestamp_expr_no_arg() {


Previously this plan would panic at runtime (because to_timestamp expects a single argument). Now it panics at plan time. Since to_timestamp() is illformed I didn't think this was a valuable test so I removed it

alamb · 2021-10-21T13:58:02Z

datafusion/src/optimizer/utils.rs

+}
+
+impl ExprRewriter for ConstEvaluator {
+    fn pre_visit(&mut self, expr: &Expr) -> Result<RewriteRecursion> {


Here is the guts of the logic for expression evaluation. I am quite pleased with how concise it is

alamb · 2021-10-21T13:58:16Z

datafusion/src/optimizer/utils.rs

+        let ctx_state = ExecutionContextState::new();
+        let input_schema = DFSchema::empty();
+
+        // The dummy column name uis used doesn't matter as only scalar


This comes directly from @pjmore ❤️

alamb · 2021-10-21T13:58:37Z

datafusion/src/optimizer/utils.rs

+            Expr::Alias(..) => false,
+            Expr::AggregateFunction { .. } => false,
+            Expr::AggregateUDF { .. } => false,
+            // TODO handle in constantant propagator pass


will file follow on ticket

alamb · 2021-10-21T13:59:53Z

datafusion/src/optimizer/utils.rs

+        // parenthesization matters: can't rewrite
+        // (rand() + 1) + 2 --> (rand() + 1) + 2)


This is an interesting one -- and something perhaps the Simplifer should do (use associative rules to rewrite rand() + 1) + 2 into rand() + (1 + 2) and then let the evaluator constant fold it

Tracked this improvement in #1162

alamb · 2021-10-21T14:00:08Z

datafusion/tests/sql.rs

 };
 use datafusion::{execution::context::ExecutionContext, physical_plan::displayable};

-/// A macro to assert that one string is contained within another with


moved into test_utils

alamb · 2021-10-21T16:25:47Z

datafusion/src/optimizer/constant_folding.rs

+
+        // TODO constant folder hould be able to run again and fold
+        // this whole thing down
+        // TODO add ticket


Suggested change

// TODO add ticket

// https://github.com/apache/arrow-datafusion/issues/1160

should we commit this issue into the PR branch?

alamb · 2021-10-23T11:44:34Z

FYI @rdettai @pjmore @Dandandan @houqp this PR is ready for review

pjmore · 2021-10-25T04:47:22Z

@rdettai made a good point on my initial implementation about how stable functions should be evaluatable in a constant context. Maybe the constant folding optimizer should have a flag to allow for evaluating stable functions in a constant context if provided execution properties? This might be useful even if it's just to make the option available to the partition pruning logic. Doing this would allow the special case for now in the expression simplifier to be removed. While this doesn't matter much at the moment because it is the only stable function if more stable built-ins are added this might start mattering. Besides the question of constant evaluating stable functions this looks good to me,

datafusion/src/optimizer/utils.rs

rdettai · 2021-10-25T10:06:48Z

datafusion/src/optimizer/utils.rs

+    fn volatility_ok(volatility: Volatility) -> bool {
+        match volatility {
+            Volatility::Immutable => true,
+            // To evaluate stable functions, need ExecutionProps, see


What is the reason not to provide ExecutionProps?

My rationale was to minimize the changes to datafusion/src/optimizer/constant_folding.rs and keep this PR smaller to review. I agree it makes sense to do now() constant folding in the simplifier.

Filed #1175 to track

In #1176 (built on this PR)

alamb · 2021-10-25T18:44:21Z

@pjmore

@rdettai made a good point on my initial implementation about how stable functions should be evaluatable in a constant context.

I think you are both right -- I will do so as a follow on PR under the aegis of #1175

alamb · 2021-10-27T10:37:38Z

@houqp / @rdettai would you be ok with my merging this PR (it does not yet have any approvals) as I have a few others backed up waiting on it to merge?

rdettai · 2021-10-27T11:57:02Z

datafusion/src/optimizer/constant_folding.rs

+        let plan = LogicalPlanBuilder::from(table_scan)
+            .filter(
+                now_expr()
+                    .lt(cast_to_int64_expr(to_timestamp_expr(ts_string)) + lit(50000)),
+            )
+            .unwrap()
+            .build()
+            .unwrap();
+
+        // Note that constant folder should be able to run again and fold
+        // this whole expression down to a single constant;
+        // https://github.com/apache/arrow-datafusion/issues/1160
+        let expected = "Filter: TimestampNanosecond(1599566400000000000) < CAST(totimestamp(Utf8(\"2020-09-08T12:05:00+00:00\")) AS Int64) + Int32(50000)\
+                        \n  TableScan: test projection=None";
+        let actual = get_optimized_plan_formatted(&plan, &time);


Isn't the ConstEvaluator supposed to solve this in one run as the expression in constant?

never mind, let's have this conversation in #1160 😉

Conversation in #1160 (comment) 👍

github-actions bot added the datafusion label Oct 20, 2021

alamb mentioned this pull request Oct 20, 2021

WIP: Alternate implementation of partial evaluation / generalized constant folding #1145

Closed

Generic constant expression evaluation

55b6c1e

alamb force-pushed the alamb/constant_eval branch from 2fdadd9 to 55b6c1e Compare October 21, 2021 13:56

alamb commented Oct 21, 2021

View reviewed changes

alamb changed the title ~~Generic constant expression evaluation (WIP)~~ Generic constant expression evaluation Oct 21, 2021

alamb marked this pull request as ready for review October 21, 2021 14:00

Better list of evaluatable expressions

da9f3d8

alamb commented Oct 21, 2021

View reviewed changes

This was referenced Oct 21, 2021

Add additional algebraic simplifications #1162

Closed

(WIP) Cleanup: remove redundant constant evaluation in Simplifier #1164

Closed

alamb requested a review from houqp October 23, 2021 11:44

alamb removed the request for review from houqp October 23, 2021 11:44

houqp requested a review from Dandandan October 25, 2021 07:06

houqp reviewed Oct 25, 2021

View reviewed changes

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

rdettai reviewed Oct 25, 2021

View reviewed changes

alamb mentioned this pull request Oct 25, 2021

Move code to fold now() from Simplifier() to ConstEvaluator #1175

Closed

alamb added 3 commits October 25, 2021 14:45

Merge remote-tracking branch 'apache/master' into alamb/constant_eval

296d96a

Fixup comments

da4f1ad

Use Null type

b2d0465

alamb mentioned this pull request Oct 25, 2021

Move code to fold Stable functions like now() from Simplifier to ConstEvaluator #1176

Merged

rdettai approved these changes Oct 27, 2021

View reviewed changes

rdettai reviewed Oct 27, 2021

View reviewed changes

alamb merged commit 156ebff into apache:master Oct 27, 2021

alamb deleted the alamb/constant_eval branch October 27, 2021 23:19

houqp added the enhancement New feature or request label Nov 6, 2021

		// parenthesization matters: can't rewrite
		// (rand() + 1) + 2 --> (rand() + 1) + 2)

	// TODO add ticket
	// https://github.com/apache/arrow-datafusion/issues/1160

Generic constant expression evaluation #1153

Generic constant expression evaluation #1153

Uh oh!

Conversation

alamb commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Follow on work

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 23, 2021

Uh oh!

pjmore commented Oct 25, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 25, 2021

Uh oh!

alamb commented Oct 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdettai Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alamb commented Oct 20, 2021 •

edited

Loading

rdettai Oct 27, 2021 •

edited

Loading