houqp commented on a change in pull request #952:
URL: https://github.com/apache/arrow-datafusion/pull/952#discussion_r698152370
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -76,15 +75,25 @@ fn create_function_physical_name(
Ok(format!("{}({}{})", fun, distinct_str, names.join(",")))
}
-fn physical_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
+fn physical_name(e: &Expr) -> Result<String> {
+ create_physical_name(e, true)
+}
+
+fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
Review comment:
nitpick, couple other naming suggestions for the `is_first_expr`
argument:
* is_root_expr
* strip_qualifer
* strip_relation
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -76,15 +75,25 @@ fn create_function_physical_name(
Ok(format!("{}({}{})", fun, distinct_str, names.join(",")))
}
-fn physical_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
+fn physical_name(e: &Expr) -> Result<String> {
Review comment:
nitpick, by just looking at the function names, it's not easy to tell
the difference between `physical_name` and `create_physical_name`. Perhaps
rename `physical_name` to `root_physical_name`?
Alternatively, we could get rid of the wrapper, then use `physical_name` and
explicitly pass in the `is_first_expr` argument everywhere.
--
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]
