Generalized constant folding #1128

pjmore · 2021-10-16T00:11:03Z

Which issue does this PR close?

Closes #1070.

What changes are included in this PR?

Added an evaluate function which evaluates literal scalar expressions using the existing physical expression implementation. Reworked ConstantFolding optimizer using the evaluate function to perform more generalized constant folding. The optimizer now traverses the expression tree depth first returning whether the expression is made up of literal expressions. When an expression that has multiple children is encountered all children expressions are checked to see if they are literals, if so then it simply returns that the expression is literal. If one of the child expressions is not literal then all literal child expressions are evaluated and then non literal is returned. For expressions that can never be evaluated in a scalar manner, such as aggregate or window expressions, the children are traversed but the expression itself will never be evaluated.

Are there any user-facing changes?

The added evaluate function is currently public to allow usage of the function from other crates in the workspace.

…ed constant folding optimizer to allow for more general folding

datafusion/src/physical_plan/expressions/mod.rs

… literal

houqp · 2021-10-17T23:59:43Z

@pjmore is this the last missing piece before you can resume the tokomak work?

pjmore · 2021-10-18T01:48:34Z

@houqp Yep, that is everything! I'm going to start on that now.

rdettai

Very interesting work! Thanks a lot!

rdettai · 2021-10-18T07:56:56Z

datafusion/src/optimizer/utils.rs

+}
+
+///Evaluates an expression if it only contains literal expressions. If a non-literal expression or non-immutable function is found then it returns an error.
+pub fn evalute_const_expr(expr: &Expr) -> Result<ScalarValue> {


Do you plan to use this method in a PR soon? Otherwise don't expose this function if it is never used (YAGNI).

Same would apply to expr_is_const() that is only used here 😃.

If you do plan to use it in a follow up PR, I would say that it would be very important to test that expr_is_const() gives consistent results with rewrite_const_expr() (If I understand the meaning of the return value correctly)

rdettai · 2021-10-18T07:57:18Z

datafusion/src/optimizer/utils.rs

+}
+
+///Evaluates an expression. Note that this will return incorrect results if the expression contains function calls which change return values call to call, such as Now().
+pub fn evaluate_const_expr_unchecked(expr: &Expr) -> Result<ScalarValue> {


I understand this is somewhat covered by the tests in constant_folding.rs, but if it is exposed publicly it should have its own set of tests.

nit: also it is surprising that this method uses fully qualified syntax (like std::sync::Arc) almost everywhere 😃

rdettai

Sorry for the review in 2 parts 😅.

rdettai · 2021-10-18T09:10:04Z

datafusion/src/optimizer/utils.rs

+    let dummy_df_schema = DFSchema::empty();
+    let dummy_input_schema = arrow::datatypes::Schema::new(vec![Field::new(
+        DUMMY_COL_NAME,
+        DataType::Float64,


can we maybe use the Null type here to make it clearer that it doesn't matter?

Suggested change

DataType::Float64,

DataType::Null,

rdettai · 2021-10-18T09:13:05Z

datafusion/src/optimizer/constant_folding.rs

+        expr
+    }
+
+    fn rewrite_const_expr(&mut self, expr: &mut Expr) -> bool {


Could we maybe comment on the meaning of the returned boolean value? If I understand correctly, it indicates whether the expression can be evaluated as a constant expression, right?

Yeah that is what it is, I'll add some documentation.

rdettai · 2021-10-18T09:19:02Z

datafusion/src/optimizer/constant_folding.rs

+                        false
+                    }
+                    (false, true) => {
+                        self.const_fold_list_eager(list);


In this case we run rewrite_const_expr() twice on each expression, is that expected?

Well rereading the code it's expected but not intended haha.

rdettai · 2021-10-18T09:22:17Z

datafusion/src/optimizer/utils.rs

+}
+
+///Evaluates an expression. Note that this will return incorrect results if the expression contains function calls which change return values call to call, such as Now().
+pub fn evaluate_const_expr_unchecked(expr: &Expr) -> Result<ScalarValue> {


nit: also it is surprising that this method uses fully qualified syntax (like std::sync::Arc) almost everywhere 😃

rdettai

Yet another question, sorry for that 😃. In case you are wondering, I am looking into to this PR thoroughly because I need something somewhat similar in #1141 to execute a partial filtering expression on the partition values: https://github.com/apache/arrow-datafusion/blob/ecb158a9fb86c52e97edb01d345d4edcb84dcefe/datafusion/src/datasource/listing.rs#L346-L428

rdettai · 2021-10-18T13:59:47Z

datafusion/src/optimizer/constant_folding.rs

            Expr::ScalarFunction {
                fun: BuiltinScalarFunction::Now,
                ..
-            } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
-                self.execution_props
-                    .query_execution_start_time
-                    .timestamp_nanos(),
-            ))),
-            Expr::ScalarFunction {
-                fun: BuiltinScalarFunction::ToTimestamp,
-                args,
            } => {
-                if !args.is_empty() {
-                    match &args[0] {
-                        Expr::Literal(ScalarValue::Utf8(Some(val))) => {
-                            match string_to_timestamp_nanos(val) {
-                                Ok(timestamp) => Expr::Literal(
-                                    ScalarValue::TimestampNanosecond(Some(timestamp)),
-                                ),
-                                _ => Expr::ScalarFunction {
-                                    fun: BuiltinScalarFunction::ToTimestamp,
-                                    args,
-                                },
-                            }
+                *expr = Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+                    self.execution_props
+                        .query_execution_start_time
+                        .timestamp_nanos(),
+                )));
+                true
+            }
+            Expr::ScalarFunction { fun, args } => {
+                if args.is_empty() {
+                    false
+                } else {
+                    let volatility = fun.volatility();
+                    match volatility {
+                        Volatility::Immutable => self.const_fold_list(args),
+                        _ => {
+                            self.const_fold_list_eager(args);
+                            false
                        }
-                        _ => Expr::ScalarFunction {
-                            fun: BuiltinScalarFunction::ToTimestamp,
-                            args,
-                        },
                    }
+                }
+            }


Could you please explain why we isolate the case of Now here instead of considering Volatility::Stable ScalarFunctions as foldable? Stable functions are supposed to be constant in the context of the query, isn't that enough?

I could have sworn that now() was implemented by rewriting the expression tree but it looks like that is wrong. You're right stable functions should be foldable.

alamb · 2021-10-18T14:15:55Z

I will review this later today -- thank you @pjmore !

alamb · 2021-10-18T19:51:26Z

datafusion/src/optimizer/utils.rs

+        &dummy_input_schema,
+        &ctx_state,
+    )?;
+    let col = {


I think you could use new_null_array here from arrow:
https://docs.rs/arrow/6.0.0/arrow/array/fn.new_null_array.html

Dandandan · 2021-10-18T20:00:24Z

datafusion/src/optimizer/constant_folding.rs

+    }
+    ///This attempts to simplify expressions of the form col(Boolean) = Boolean and col(Boolean) != Boolean
+    /// e.g. col(Boolean) = Some(true) -> col(Boolean). It also handles == and != between two boolean literals as
+    /// the binary operator physical expression currently doesn't handle them.


Is it possible to move this to the binary operator then?

I think if we take the approach on #1145, we can remove this code that handles constants

I like that your approach uses the expression rewriting framework that's already in place

alamb

Thanks @pjmore -- this is pretty awesome.

One thing I was thinking when looking at this PR, was that the partial evaluation / rewrites and and constant folding were somewhat intertwined.

What would you think of extracting the partial evaluator into a function that could be invoked by the constant folding pass rather than something that is part of the constant folding pass?

I sketched out what this might look like in this PR here #1145 (I am happy to finish that up if you think it is valuable)

I was thinking then we could encode rules like

false OR <expr> --> <expr>
false AND <expr> --> false

as part of the constant folder and have the constant folder invoke the evaluator.

What do you think?

alamb · 2021-10-18T20:14:07Z

datafusion/src/optimizer/utils.rs

+    }
+}
+
+///Evaluates an expression if it only contains literal expressions. If a non-literal expression or non-immutable function is found then it returns an error.


I suggest the public API should be something like:

partially_evaluate_expr(expr: &Expr)

or

Expr::partially_evaluate(self) -> Self

pjmore · 2021-10-19T23:07:57Z

Yeah I think that's is a better approach. Those special cases with Expr::Not and Expr::BinaryExpr are not pretty in my implementation. Given that the vast majority of this PR can be deleted if we go that route I'm going to close this.

alamb · 2021-10-20T10:33:56Z

Yeah I think that's is a better approach. Those special cases with Expr::Not and Expr::BinaryExpr are not pretty in my implementation. Given that the vast majority of this PR can be deleted if we go that route I'm going to close this.

Cool -- thank you @pjmore for pushing this forward. Would you like to keep working at it, or shall I polish up #1145 to get that merged?

This feature is going to really push DataFusion forward I think

pjmore added 5 commits October 13, 2021 22:33

Added an evaluate function to evaluate literal expressions and extend…

1b4af9a

…ed constant folding optimizer to allow for more general folding

formatted code

18d966e

Removed ExecutionProps argument for evaluate

b943a18

Fixed alias not being preserved

2ad8480

Moved evaluate function

a87e8c7

github-actions bot added the datafusion Changes in the datafusion crate label Oct 16, 2021

houqp reviewed Oct 16, 2021

View reviewed changes

datafusion/src/physical_plan/expressions/mod.rs Outdated Show resolved Hide resolved

Moved and renamed evalute function. Added function to test if expr is…

81972d5

… literal

houqp approved these changes Oct 17, 2021

View reviewed changes

houqp requested review from alamb, jorgecarleitao and Dandandan October 17, 2021 23:56

houqp added the enhancement New feature or request label Oct 17, 2021

rdettai suggested changes Oct 18, 2021

View reviewed changes

rdettai reviewed Oct 18, 2021

View reviewed changes

alamb reviewed Oct 18, 2021

View reviewed changes

Dandandan reviewed Oct 18, 2021

View reviewed changes

alamb mentioned this pull request Oct 18, 2021

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

Closed

alamb reviewed Oct 18, 2021

View reviewed changes

pjmore closed this Oct 19, 2021

alamb mentioned this pull request Oct 20, 2021

Generic constant expression evaluation #1153

Merged

rdettai mentioned this pull request Oct 27, 2021

Support simplification that requires multiple applications of constant folding / simplification #1160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalized constant folding #1128

Generalized constant folding #1128

pjmore commented Oct 16, 2021

houqp commented Oct 17, 2021

pjmore commented Oct 18, 2021

rdettai left a comment

rdettai Oct 18, 2021 •

edited

rdettai Oct 18, 2021

rdettai Oct 18, 2021 •

edited

rdettai left a comment

rdettai Oct 18, 2021

rdettai Oct 18, 2021

pjmore Oct 19, 2021

rdettai Oct 18, 2021

pjmore Oct 19, 2021

rdettai Oct 18, 2021 •

edited

rdettai left a comment •

edited

rdettai Oct 18, 2021

pjmore Oct 19, 2021

alamb commented Oct 18, 2021

alamb Oct 18, 2021 •

edited

Dandandan Oct 18, 2021

alamb Oct 18, 2021

pjmore Oct 19, 2021

alamb left a comment •

edited

alamb Oct 18, 2021

pjmore commented Oct 19, 2021

alamb commented Oct 20, 2021 •

edited

Generalized constant folding #1128

Generalized constant folding #1128

Conversation

pjmore commented Oct 16, 2021

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

houqp commented Oct 17, 2021

pjmore commented Oct 18, 2021

rdettai left a comment

Choose a reason for hiding this comment

rdettai Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdettai Oct 18, 2021 • edited

Choose a reason for hiding this comment

rdettai 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

rdettai Oct 18, 2021 • edited

Choose a reason for hiding this comment

rdettai left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Oct 18, 2021

alamb Oct 18, 2021 • edited

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 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjmore commented Oct 19, 2021

alamb commented Oct 20, 2021 • edited

rdettai Oct 18, 2021 •

edited

rdettai Oct 18, 2021 •

edited

rdettai Oct 18, 2021 •

edited

rdettai left a comment •

edited

alamb Oct 18, 2021 •

edited

alamb left a comment •

edited

alamb commented Oct 20, 2021 •

edited