alamb commented on code in PR #7785:
URL: https://github.com/apache/arrow-datafusion/pull/7785#discussion_r1352452459
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1060,7 +1089,7 @@ impl LogicalPlan {
}
impl LogicalPlan {
- /// applies collect to any subqueries in the plan
+ /// applies `op` to any subqueries in the plan
Review Comment:
drive by cleanups
##########
datafusion/core/src/dataframe.rs:
##########
@@ -1218,7 +1218,39 @@ impl DataFrame {
Ok(DataFrame::new(self.session_state, project_plan))
}
- /// Convert a prepare logical plan into its inner logical plan with all
params replaced with their corresponding values
+ /// Replace all parameters in logical plan with the specified
+ /// values, in preparation for execution.
+ ///
+ /// # Example
+ ///
+ /// ```
+ /// use datafusion::prelude::*;
+ /// # use datafusion::{error::Result, assert_batches_eq};
+ /// # #[tokio::main]
+ /// # async fn main() -> Result<()> {
+ /// # use datafusion_common::ScalarValue;
+ /// let mut ctx = SessionContext::new();
+ /// # ctx.register_csv("example", "tests/data/example.csv",
CsvReadOptions::new()).await?;
+ /// let results = ctx
+ /// .sql("SELECT a FROM example WHERE b = $1")
+ /// .await?
+ /// // replace $1 with value 2
+ /// .with_param_values(vec![ScalarValue::from(2i64)])?
Review Comment:
this tests the DataFrame API and fails without the changes to
`LogicalPlan::with_param_values`
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2767,10 +2806,7 @@ digraph {
let plan = table_scan(TableReference::none(), &schema, None)
.unwrap()
- .filter(col("id").eq(Expr::Placeholder(Placeholder::new(
- "".into(),
- Some(DataType::Int32),
- ))))
+ .filter(col("id").eq(placeholder("")))
Review Comment:
here is an example of the new `placeholder` function being much nicer to use
##########
datafusion/expr/src/expr.rs:
##########
@@ -1030,6 +1034,49 @@ impl Expr {
pub fn contains_outer(&self) -> bool {
!find_out_reference_exprs(self).is_empty()
}
+
+ /// Find all [`Expr::Placeholder`] in anthis, and try
+ /// to infer their [`DataType`] from the context of their use.
+ pub fn infer_placeholder_types(self, schema: &DFSchema) -> Result<Expr> {
Review Comment:
This was moved from the `datafusion-sql` module as it is not SQL specific,
but generic to expressions
##########
datafusion/expr/src/expr_fn.rs:
##########
@@ -80,6 +80,22 @@ pub fn ident(name: impl Into<String>) -> Expr {
Expr::Column(Column::from_name(name))
}
+// Create placeholder value that will be filled in (such as `$1`):
+///
+/// For example:
+///
+/// ```rust
+/// # use datafusion_expr::{placeholder};
Review Comment:
I think the example is key here to demonstrate the "$" is included (which
confused me for a bit)
--
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]
