[GitHub] [arrow-datafusion] houqp commented on a change in pull request #952: Change compound column field name rules

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org