jonahgao commented on code in PR #14669:
URL: https://github.com/apache/datafusion/pull/14669#discussion_r1957125726
##########
datafusion/expr/src/planner.rs:
##########
@@ -62,39 +75,44 @@ pub trait ContextProvider {
not_impl_err!("Recursive CTE is not implemented")
}
- /// Getter for expr planners
+ /// Return [`ExprPlanner`] extensions for planning expressions
fn get_expr_planners(&self) -> &[Arc<dyn ExprPlanner>] {
&[]
}
- /// Getter for the data type planner
+ /// Return [`TypePlanner`] extensions for planning data types
fn get_type_planner(&self) -> Option<Arc<dyn TypePlanner>> {
None
}
- /// Getter for a UDF description
+ /// Return the scalar function with a given name, if any
fn get_function_meta(&self, name: &str) -> Option<Arc<ScalarUDF>>;
- /// Getter for a UDAF description
+
+ /// Return the aggregate function with a given name, if any
fn get_aggregate_meta(&self, name: &str) -> Option<Arc<AggregateUDF>>;
- /// Getter for a UDWF
+
+ /// Return the window function with a given name, if any
fn get_window_meta(&self, name: &str) -> Option<Arc<WindowUDF>>;
- /// Getter for system/user-defined variable type
+
+ /// Return the system/user-defined variable type, if any
+ ///
+ /// A user defined variable is typically accessed via `@@var_name`
Review Comment:
```suggestion
/// A user defined variable is typically accessed via `@var_name`
```
`@@var_name` should be a system variable, according to
[is_system_variables()](https://github.com/apache/datafusion/blob/1626c00a1b2f14057c8c4357ef9ea349350bbb60/datafusion/expr/src/var_provider.rs#L41)
##########
datafusion/expr/src/planner.rs:
##########
@@ -49,11 +54,19 @@ pub trait ContextProvider {
not_impl_err!("Table Functions are not supported")
}
- /// This provides a worktable (an intermediate table that is used to store
the results of a CTE during execution)
- /// We don't directly implement this in the logical plan's ['SqlToRel`]
- /// because the sql code needs access to a table that contains
execution-related types that can't be a direct dependency
- /// of the sql crate (namely, the `CteWorktable`).
+ /// Provides an intermediate table that is used to store the results of a
CTE during execution)
+ ///
+ /// CTE stands for "Common Table Expression"
+ ///
+ /// # Notes
+ /// We don't directly implement this in ['SqlToRel`] as implementing this
function
Review Comment:
This paragraph is not well formatted.
##########
datafusion/sql/src/planner.rs:
##########
@@ -224,7 +224,24 @@ impl PlannerContext {
}
}
-/// SQL query planner
+/// SQL query planner and binder
+///
+/// This struct is used to convert a SQL AST into a [`LogicalPlan`].
+///
+/// You can control the behavior of the planner by providing [`ParserOptions`].
+///
+/// It performs the following tasks:
+///
+/// 1. Name and type resolution (called "binding" in other systems). This
+/// phase looks up table and column names using the [`ContextProvider`].
+/// 2. Mechanical translation of the AST into a [`LogicalPlan`].
+///
+/// It does not perform type checking, semantic analysis, type coercion, or
Review Comment:
I'm not sure if
[check_unnest_arg](https://github.com/apache/datafusion/blob/1626c00a1b2f14057c8c4357ef9ea349350bbb60/datafusion/sql/src/expr/function.rs#L450C19-L450C35)
conflicts with the description of type checking.
##########
datafusion/sql/src/planner.rs:
##########
@@ -224,7 +224,24 @@ impl PlannerContext {
}
}
-/// SQL query planner
+/// SQL query planner and binder
Review Comment:
As far as I know, they are accurate.
##########
datafusion/core/src/lib.rs:
##########
@@ -185,7 +185,7 @@
//! specialize any behavior for your use case. For example,
//! some projects may add custom [`ExecutionPlan`] operators, or create their
own
//! query language that directly creates [`LogicalPlan`] rather than using the
-//! built in SQL planner, [`SqlToRel`].
+//! provided in SQL planner, [`SqlToRel`].
Review Comment:
“builtin” is clearer to me.
##########
datafusion/expr/src/planner.rs:
##########
@@ -49,11 +54,19 @@ pub trait ContextProvider {
not_impl_err!("Table Functions are not supported")
}
- /// This provides a worktable (an intermediate table that is used to store
the results of a CTE during execution)
- /// We don't directly implement this in the logical plan's ['SqlToRel`]
- /// because the sql code needs access to a table that contains
execution-related types that can't be a direct dependency
- /// of the sql crate (namely, the `CteWorktable`).
+ /// Provides an intermediate table that is used to store the results of a
CTE during execution)
Review Comment:
```suggestion
/// Provides an intermediate table that is used to store the results of
a CTE during execution
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
