You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/05/01 17:11:32 UTC
[PR] Add `SessionContext::create_physical_expr()` and `SessionState::create_physical_expr()` [datafusion]
alamb opened a new pull request, #10330:
URL: https://github.com/apache/datafusion/pull/10330
## Which issue does this PR close?
Closes https://github.com/apache/datafusion/issues/10181
## Rationale for this change
Rationale:
There important use cases for creating `Expr`s 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 https://github.com/apache/datafusion/issues/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
1. Add `SessionContext::create_physical_expr()` and `SessionState::create_physical_expr()`
2. (TODO) Add function rewrites
4. Update examples to use that
4. (TODO) Add tests for the new API
## Are these changes tested?
Yes, new tests
## Are there any user-facing changes?
Yes: New API, changed XXX
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#issuecomment-2097323808
@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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1586867994
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -510,6 +515,34 @@ impl SessionContext {
}
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
Review Comment:
```suggestion
/// coercion and function rewrites.
```
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -510,6 +515,34 @@ impl SessionContext {
}
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// # Example
+ /// ```
+ /// # use std::sync::Arc;
+ /// # use arrow::datatypes::{DataType, Field, Schema};
+ /// # use datafusion::prelude::*;
+ /// # use datafusion_common::DFSchema;
+ /// // a = 1 (i64)
+ /// let expr = col("a").eq(lit(1i64));
+ /// // 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`
Review Comment:
Nice, we have some (probably duplicated) type coercion logic on our side we might be able to replace.
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
Review Comment:
Why not?
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
+ ///
+ /// # See Also:
+ /// * [`SessionContext::create_physical_expr`] for a higher-level API
Review Comment:
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](https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionContext.html) can be created from a [SessionConfig](https://docs.rs/datafusion/latest/datafusion/prelude/struct.SessionConfig.html) and stores the state for a particular query session. A single [SessionContext](https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionContext.html) can run multiple queries.
> [SessionState](https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionState.html) contains information available during query planning (creating [LogicalPlan](https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.LogicalPlan.html)s and [ExecutionPlan](https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html)s).
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).
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -2024,6 +2089,35 @@ impl SessionState {
}
}
+struct SessionSimpifyProvider<'a> {
+ state: &'a SessionState,
+ df_schema: &'a DFSchema,
+}
+
+impl<'a> SessionSimpifyProvider<'a> {
Review Comment:
```suggestion
impl<'a> SessionSimplifyProvider<'a> {
```
##########
datafusion/common/src/dfschema.rs:
##########
@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
}
}
+impl AsRef<Schema> for DFSchema {
Review Comment:
:exploding_head:
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1587512649
##########
datafusion/common/src/dfschema.rs:
##########
@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
}
}
+impl AsRef<Schema> for DFSchema {
Review Comment:
yeah, https://github.com/apache/datafusion/pull/9595 was epic -- kudos to @haohuaijin for making that happen
##########
datafusion/common/src/dfschema.rs:
##########
@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
}
}
+impl AsRef<Schema> for DFSchema {
Review Comment:
yeah, https://github.com/apache/datafusion/pull/9595 was epic -- kudos to @haohuaijin and @matthewmturner for making that happen
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1587641072
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
+ ///
+ /// # See Also:
+ /// * [`SessionContext::create_physical_expr`] for a higher-level API
Review Comment:
Made https://github.com/apache/datafusion/pull/10350 to hopefully clarify
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1586971614
##########
datafusion/common/src/dfschema.rs:
##########
@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
}
}
+impl AsRef<Schema> for DFSchema {
Review Comment:
Very nice!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1587485998
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -2024,6 +2089,35 @@ impl SessionState {
}
}
+struct SessionSimpifyProvider<'a> {
+ state: &'a SessionState,
+ df_schema: &'a DFSchema,
+}
+
+impl<'a> SessionSimpifyProvider<'a> {
Review Comment:
🤦
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #10330:
URL: https://github.com/apache/datafusion/pull/10330
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#issuecomment-2097355264
> @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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add APIs to create `PhysicalExpressions` from `Expr`s: `SessionContext::create_physical_expr()` and `SessionState::create_physical_expr()` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1586649331
##########
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())?;
Review Comment:
This shows how the new API is used - `SessionContext::new().create_physical_expr`
##########
datafusion/common/src/dfschema.rs:
##########
@@ -806,6 +820,12 @@ impl From<&DFSchema> for Schema {
}
}
+impl AsRef<Schema> for DFSchema {
Review Comment:
This allows DFSchema to be treated like a &Schema, which is now possible after https://github.com/apache/datafusion/pull/9595
##########
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>> {
Review Comment:
This PR basically ports this code out of the examples and into `SessionState` and adds documentation and tests
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -510,6 +515,34 @@ impl SessionContext {
}
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
Review Comment:
New public API with example
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -2024,6 +2089,35 @@ impl SessionState {
}
}
+struct SessionSimpifyProvider<'a> {
Review Comment:
This avoids cloning schema / schema refs
##########
datafusion/core/tests/expr_api/mod.rs:
##########
@@ -0,0 +1,181 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::util::pretty::pretty_format_columns;
+use arrow_array::builder::{ListBuilder, StringBuilder};
+use arrow_array::{ArrayRef, RecordBatch, StringArray, StructArray};
+use arrow_schema::{DataType, Field};
+use datafusion::prelude::*;
+use datafusion_common::DFSchema;
+/// Tests of using and evaluating `Expr`s outside the context of a LogicalPlan
+use std::sync::{Arc, OnceLock};
+
+#[test]
+fn test_eq() {
+ // id = '2'
+ evaluate_expr_test(
+ col("id").eq(lit("2")),
+ vec![
+ "+-------+",
+ "| expr |",
+ "+-------+",
+ "| false |",
+ "| true |",
+ "| false |",
+ "+-------+",
+ ],
+ );
+}
+
+#[test]
+fn test_eq_with_coercion() {
+ // id = 2 (need to coerce the 2 to '2' to evaluate)
+ evaluate_expr_test(
+ col("id").eq(lit(2i32)),
+ vec![
+ "+-------+",
+ "| expr |",
+ "+-------+",
+ "| false |",
+ "| true |",
+ "| false |",
+ "+-------+",
+ ],
+ );
+}
+
+#[test]
+fn test_get_field() {
Review Comment:
this tests fail without function rewrites applied
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
+ ///
+ /// # See Also:
+ /// * [`SessionContext::create_physical_expr`] for a higher-level API
+ /// * [`create_physical_expr`] for a lower-level API
+ ///
+ /// [simplified]: datafusion_optimizer::simplify_expressions
+ pub fn create_physical_expr(
+ &self,
+ expr: Expr,
+ df_schema: &DFSchema,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ let simplifier =
+ ExprSimplifier::new(SessionSimpifyProvider::new(self, df_schema));
+ // apply type coercion here to ensure types match
+ let mut expr = simplifier.coerce(expr, df_schema)?;
+
+ // rewrite Exprs to functions if necessary
+ let config_options = self.config_options();
Review Comment:
Applying these rewrites is a new feature (and what actually closes https://github.com/apache/datafusion/issues/10181)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1587497942
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
+ ///
+ /// # See Also:
+ /// * [`SessionContext::create_physical_expr`] for a higher-level API
Review Comment:
I agree this is confusing -- I will write up some documentation about this
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#discussion_r1587503305
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1947,6 +1982,36 @@ impl SessionState {
.await
}
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not [simplified]
Review Comment:
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](https://docs.rs/datafusion/latest/datafusion/optimizer/unwrap_cast_in_comparison/index.html)" 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `SessionContext`/`SessionState::create_physical_expr()` to create `PhysicalExpressions` from `Expr`s [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10330:
URL: https://github.com/apache/datafusion/pull/10330#issuecomment-2097919890
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.
<img width="1060" alt="Screenshot 2024-05-07 at 5 44 54 AM" src="https://github.com/apache/datafusion/assets/490673/30ac8a07-d726-410b-b269-0a95ed58cfeb">
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org