Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s #10330

alamb · 2024-05-01T17:11:29Z

~~Draft as it builds on #10331~~

Which issue does this PR close?

Rationale for this change

Rationale:

There important use cases for creating Exprs and executing them as a
PhysicalExpr outside the context of a query (for example, to apply delete
predicates in delta.rs)

At the moment this is possible but is often tricky as users have to manually
invoke type coercion and simplification rules to get the Expr into a form that
can be executed (see example here)

After #8045, to use certain expressions that translate to functions it is also important to
to apply function rewrite rules. See #10181 for more details

Thus having a proper, tested, documented public API that does this I think is
valuable and will prevent future regressions as we continue refactoring.

What changes are included in this PR?

Changes

Add SessionContext::create_physical_expr() and SessionState::create_physical_expr()
Apply FunctionRewrites
Update examples to use the new API
Add tests for the new API

Are these changes tested?

Yes, new tests

Are there any user-facing changes?

Yes: New API

…e_physical_expr()`

alamb · 2024-05-01T18:51:59Z

datafusion-examples/examples/expr_api.rs

@@ -92,7 +90,8 @@ fn evaluate_demo() -> Result<()> {
    let expr = col("a").lt(lit(5)).or(col("a").eq(lit(8)));

    // First, you make a "physical expression" from the logical `Expr`
-    let physical_expr = physical_expr(&batch.schema(), expr)?;
+    let df_schema = DFSchema::try_from(batch.schema())?;


This shows how the new API is used - SessionContext::new().create_physical_expr

alamb · 2024-05-01T18:52:23Z

datafusion-examples/examples/expr_api.rs

@@ -248,21 +251,6 @@ fn make_ts_field(name: &str) -> Field {
    make_field(name, DataType::Timestamp(TimeUnit::Nanosecond, tz))
 }

-/// Build a physical expression from a logical one, after applying simplification and type coercion
-pub fn physical_expr(schema: &Schema, expr: Expr) -> Result<Arc<dyn PhysicalExpr>> {


This PR basically ports this code out of the examples and into SessionState and adds documentation and tests

alamb · 2024-05-01T18:52:54Z

datafusion/common/src/dfschema.rs

@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
    }
 }

+impl AsRef<Schema> for DFSchema {


This allows DFSchema to be treated like a &Schema, which is now possible after #9595

yeah, #9595 was epic -- kudos to @haohuaijin and @matthewmturner for making that happen

alamb · 2024-05-01T18:55:40Z

datafusion/core/src/execution/context/mod.rs

@@ -510,6 +515,34 @@ impl SessionContext {
        }
    }

+    /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type


New public API with example

alamb · 2024-05-01T18:55:59Z

datafusion/core/src/execution/context/mod.rs

@@ -2024,6 +2089,35 @@ impl SessionState {
    }
 }

+struct SessionSimpifyProvider<'a> {


This avoids cloning schema / schema refs

alamb · 2024-05-01T18:56:35Z

datafusion/core/src/execution/context/mod.rs

+        let mut expr = simplifier.coerce(expr, df_schema)?;
+
+        // rewrite Exprs to functions if necessary
+        let config_options = self.config_options();


Applying these rewrites is a new feature (and what actually closes #10181)

alamb · 2024-05-01T18:56:47Z

datafusion/core/tests/expr_api/mod.rs

+}
+
+#[test]
+fn test_get_field() {


this tests fail without function rewrites applied

westonpace

A few questions (and a typo) but I think this provides what we will need.

datafusion/core/src/execution/context/mod.rs

westonpace · 2024-05-01T21:56:01Z

datafusion/core/src/execution/context/mod.rs

+    /// // provide type information that `a` is an Int32
+    /// let schema = Schema::new(vec![Field::new("a", DataType::Int32, true)]);
+    /// let df_schema = DFSchema::try_from(schema).unwrap();
+    /// // Create a PhysicalExpr. Note DataFusion automatically coerces (casts) `1i64` to `1i32`


Nice, we have some (probably duplicated) type coercion logic on our side we might be able to replace.

westonpace · 2024-05-01T21:57:47Z

datafusion/core/src/execution/context/mod.rs

+    /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+    /// coercion, and function rewrites.
+    ///
+    /// Note: The expression is not [simplified]


My rationale was that simplification is substantially more expensive due to the rewrites required and is not strictly necessary for evaluation, so I felt not simplifying would be less surprising

Also, if we are going to do simplification, it might also be reasonable to ask "why not other optimization like comparison cast unwrapping" too

I think it is important to apply coercion as is very hard to create the right expression to get the type exactly aligned resulting in hard to fix errors. Same thing for this function rewriting.

I'll add this context to the comments in this PR

westonpace · 2024-05-01T22:01:20Z

datafusion/core/src/execution/context/mod.rs

+    /// Note: The expression is not [simplified]
+    ///
+    /// # See Also:
+    /// * [`SessionContext::create_physical_expr`] for a higher-level API


This is somewaht unrelated to this PR but It's news to me that "SessionContext" is higher level than "SessionState". The only comparison between the two that I can find is https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionContext.html#sessioncontext-sessionstate-and-taskcontext which says:

A SessionContext can be created from a SessionConfig and stores the state for a particular query session. A single SessionContext can run multiple queries.

SessionState contains information available during query planning (creating LogicalPlans and ExecutionPlans).

From this it is not obvious that a "SessionContext" contains a "SessionState". It's also very confusing when I would use "SessionContext" and when I would use "SessionState" in my user application. Currently, I've been going off the philosophy of "if an API method needs a SessionState I'd better create one and if an API method needs a SessionContext I'd better create one."

Some kind of long form design documentation on these two types would be interesting (or if something already exists that I've missed or is in a blog post somewhere that would be cool too).

I agree this is confusing -- I will write up some documentation about this

Made #10350 to hopefully clarify

datafusion/core/src/execution/context/mod.rs

westonpace · 2024-05-01T22:03:13Z

datafusion/common/src/dfschema.rs

@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
    }
 }

+impl AsRef<Schema> for DFSchema {


Co-authored-by: Weston Pace <[email protected]>

andygrove · 2024-05-07T02:40:54Z

@phillipleblanc @westonpace Do you have any further feedback on this PR? I think this is the final item we're waiting on before releasing 38.0.0

phillipleblanc · 2024-05-07T03:00:45Z

@phillipleblanc @westonpace Do you have any further feedback on this PR? I think this is the final item we're waiting on before releasing 38.0.0

Looks good to me.

alamb · 2024-05-07T10:00:09Z

Thanks for your reviews @phillipleblanc and @westonpace

@andygrove I can't merge this PR until it is approved by a committer, based on the rules in this repo, but from my perspective it is good to go.

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels May 1, 2024

alamb changed the title ~~Add SessionContext::create_physical_expr() and SessionState::create_physical_expr()~~ Add APIs to create PhysicalExpressions from Exprs: SessionContext::create_physical_expr() and SessionState::create_physical_expr() May 1, 2024

alamb force-pushed the alamb/expression_api branch from 86548e3 to cfd4440 Compare May 1, 2024 17:37

Improve coerce API so it does not need DFSchema

37bb5dc

alamb force-pushed the alamb/expression_api branch from cfd4440 to c326003 Compare May 1, 2024 18:51

Add SessionContext::create_physical_expr() and `SessionState::creat…

e896763

…e_physical_expr()`

alamb force-pushed the alamb/expression_api branch from c326003 to e896763 Compare May 1, 2024 18:58

alamb commented May 1, 2024

View reviewed changes

alamb changed the title ~~Add APIs to create PhysicalExpressions from Exprs: SessionContext::create_physical_expr() and SessionState::create_physical_expr()~~ Add SessionContext/SessionState::create_physical_expr() to create PhysicalExpressions from Exprs May 1, 2024

This was referenced May 1, 2024

Error "entered unreachable code: NamedStructField should be rewritten in OperatorToFunction" after upgrade to 37 #10181

Closed

DataFusion 38.0.0 Release #10217

Closed

westonpace reviewed May 1, 2024

View reviewed changes

alamb and others added 3 commits May 2, 2024 07:40

Apply suggestions from code review

88d0545

Co-authored-by: Weston Pace <[email protected]>

Merge remote-tracking branch 'apache/main' into alamb/expression_api

46880f6

Add note on simplification

95d6739

alamb marked this pull request as ready for review May 2, 2024 11:53

This was referenced May 2, 2024

Clarify docs explaining the relationship between SessionState and SessionContext #10350

Merged

Add example of using Expr::field in 37.1.0 #10183

Closed

DataFusion weekly project plan (Andrew Lamb) - May 6, 2024 #10395

Closed

phillipleblanc approved these changes May 7, 2024

View reviewed changes

westonpace approved these changes May 7, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/expression_api

77b27cc

andygrove approved these changes May 7, 2024

View reviewed changes

andygrove merged commit c8b8c74 into apache:main May 7, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s #10330

Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s #10330

alamb commented May 1, 2024 •

edited

Loading

alamb May 1, 2024

alamb May 1, 2024

alamb May 1, 2024

westonpace May 1, 2024

phillipleblanc May 2, 2024

alamb May 2, 2024 •

edited

Loading

alamb May 1, 2024

alamb May 1, 2024

alamb May 1, 2024

alamb May 1, 2024

westonpace left a comment

westonpace May 1, 2024

westonpace May 1, 2024

alamb May 2, 2024

westonpace May 1, 2024

alamb May 2, 2024

alamb May 2, 2024

westonpace May 1, 2024

andygrove commented May 7, 2024

phillipleblanc commented May 7, 2024

alamb commented May 7, 2024

Add SessionContext/SessionState::create_physical_expr() to create PhysicalExpressions from Exprs #10330

Add SessionContext/SessionState::create_physical_expr() to create PhysicalExpressions from Exprs #10330

Conversation

alamb commented May 1, 2024 • edited Loading

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?

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 May 2, 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

westonpace 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

Choose a reason for hiding this comment

andygrove commented May 7, 2024

phillipleblanc commented May 7, 2024

alamb commented May 7, 2024

Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s #10330

Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s #10330

alamb commented May 1, 2024 •

edited

Loading

alamb May 2, 2024 •

edited

Loading