Use `schema_name` to create the `physical_name` #11977

joroKr21 · 2024-08-14T09:59:21Z

More consistency and less opportunity for column name mismatch.

Which issue does this PR close?

Part of #11782.

Rationale for this change

It's not manageable to have these two slightly different ways of obtaining a name from an expression.
Also, minor differences in the implementation can cause schema mismatch errors.

What changes are included in this PR?

physical_name delegates to schema_name for all cases except top-level columns which are unqualified.
This was previously the purpose of the is_first_expr flag in create_physical_name.

Are these changes tested?

Relying on existing tests.

Are there any user-facing changes?

Expr::canonical_name is deprecated because it's just an alias for format!
create_function_physical_name is removed

More consistency and less opportunity for column name mismatch.

alamb

Thank you @joroKr21 -- this is an amazing PR (amazing to delete so much code but not change any tests -- that really shows a deep understanding of the code)

cc @mustafasrepo and @jayzhan211 as I vaguely remember we discussed the need for this alternate path for display expressions. Clearly as all the tests pass the second copy isn't needed it seems

alamb · 2024-08-14T21:36:45Z

datafusion/expr/src/expr.rs

@@ -1104,6 +1103,7 @@ impl Expr {
    }

    /// Returns a full and complete string representation of this expression.
+    #[deprecated(note = "use format! instead")]


alamb · 2024-08-14T21:37:12Z

datafusion/expr/src/expr.rs

+/// The name of the column (field) that this `Expr` will produce in the physical plan.
+/// The difference from [Expr::schema_name] is that top-level columns are unqualified.
+pub fn physical_name(expr: &Expr) -> Result<String> {
+    if let Expr::Column(col) = expr {


alamb · 2024-08-14T21:38:04Z

datafusion/physical-plan/src/aggregates/mod.rs

@@ -2179,6 +2179,7 @@ mod tests {
            .map(|order_by_expr| {
                let ordering_req = order_by_expr.unwrap_or_default();
                AggregateExprBuilder::new(array_agg_udaf(), vec![Arc::clone(col_a)])
+                    .alias("a")


why is this change needed?

This is the issue, we are not able to get the correct name from the args, so alias is a workaround solution

Do we really need it? Is there a situation where we would not provide the alias to the physical expression?
I made this change because the alias generated previously was incorrect (it didn't use the arguments).

jayzhan211 · 2024-08-14T23:51:46Z

We should get physical name from Physical expression not from Expr. If we are building the expr from builder, there is no Expr that you could get the name from.
Btw, if you are interesting to solve this issue, let me know, so I can work on other task

joroKr21 · 2024-08-15T05:56:34Z

If we are building the expr from builder, there is no Expr that you could get the name from.

Is there a use case for this? The way I see it, physical expressions are low-level enough that you would have to explicitly provide an alias. But I'm curious to know.

jayzhan211 · 2024-08-15T09:26:00Z

If we are building the expr from builder, there is no Expr that you could get the name from.

Is there a use case for this? The way I see it, physical expressions are low-level enough that you would have to explicitly provide an alias. But I'm curious to know.

In datafusion-comet, they heavily rely on physical layer stuffs. They directly build the physical expression with out logical expression.

you would have to explicitly provide an alias

I guess it is possible to create the name based on arguments for most of the case, and they can also alias the unreadable complex expression to a nicer one.

alamb · 2024-08-15T10:41:59Z

Maybe @andygrove / @viirya could also provide some feedback -- I don't understand the complexities of the naming requirements in comet

If there are some other non obvious requirements for comet, it would be great to get some additional tests in DataFusion that demonstrate the usecase so that we don't break something for comet accidentally.

viirya · 2024-08-15T14:59:33Z

datafusion/expr/src/expr.rs

-            internal_err!("Create physical name does not support OuterReferenceColumn")
-        }
+/// The name of the column (field) that this `Expr` will produce in the physical plan.
+/// The difference from [Expr::schema_name] is that top-level columns are unqualified.


It would be better if we can provide example like schema_name to show difference.

viirya · 2024-08-15T15:04:06Z

This was previously the purpose of the is_first_expression flag in create_physical_name.

I don't see the appearance of is_first_expression flag in the diff. So the description looks a bit confused.

viirya · 2024-08-15T15:06:09Z

Maybe @andygrove / @viirya could also provide some feedback -- I don't understand the complexities of the naming requirements in comet

If there are some other non obvious requirements for comet, it would be great to get some additional tests in DataFusion that demonstrate the usecase so that we don't break something for comet accidentally.

As Comet doesn't go through the query analysis of DataFusion, I think it should be fine from name resolution change.

joroKr21 · 2024-08-15T15:06:32Z

datafusion/expr/src/expr.rs

-    create_physical_name(e, true)
-}
-
-fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {


@viirya this was the flag, I remembered it incorrectly: is_first_expr

viirya

It looks good to have a consistent approach for physical name for expression, instead of both physical_name and schema_name.

alamb · 2024-08-19T16:17:03Z

What is the current status of this PR?

I am a little confused

@viirya seems to suggest this would be a good change, but @jayzhan211 points out this change would mean anyone who built PhysicalExprs without a corresponding Expr would have to create their own schema names

We should get physical name from Physical expression not from Expr. If we are building the expr from builder, there is no Expr that you could get the name from.

So I guess my question is "if we merge this PR would it mess up comet (or other users of only physical exprs)?"

viirya · 2024-08-19T17:38:26Z

@viirya seems to suggest this would be a good change, but @jayzhan211 points out this change would mean anyone who built PhysicalExprs without a corresponding Expr would have to create their own schema names

Hmm, as we built physical operators directly (without going through analysis stage in DataFusion), the bindings are done at Spark analysis and Comet uses the bindings when creating physical expressions and operators. So we don't rely on name resolution in DataFusion.

That's said, I assume that names of physical expressions don't matter in physical operators. The change to DataFusion analysis resolution should not have impact. Do we still have additional name resolution in physical stage in DataFusion?

jayzhan211 · 2024-08-20T03:19:09Z

Do we still have additional name resolution in physical stage in DataFusion?

The name is part of the schema and is verified against column name inside projection mapping and it is also the name displayed in explain statement

datafusion/datafusion/physical-expr/src/equivalence/projection.rs

Lines 66 to 74 in 67cf1d6

    
           let idx = col.index(); 
        
           let matching_input_field = input_schema.field(idx); 
        
           if col.name() != matching_input_field.name() { 
        
               return internal_err!("Input field name {} does not match with the projection expression {}", 
        
                   matching_input_field.name(),col.name()) 
        
               } 
        
           let matching_input_column = 
        
               Column::new(matching_input_field.name(), idx); 
        
           Ok(Transformed::yes(Arc::new(matching_input_column)))

viirya · 2024-08-20T03:31:46Z

It looks like a simple check that if a column's name matches corresponding input field name in input schema. Isn't? As we provide matched input schema and column names to projection, I think it should be all good. I'm not sure how does this PR impact us.

jayzhan211 · 2024-08-20T03:35:32Z

I'm not sure how does this PR impact us.

It will not. Merge this PR does not impact Comet.

Do we still have additional name resolution in physical stage in DataFusion?

The response for this is just to inform we still have name resolution in physical stage in Datafusion so the name of physical expressions matters and might not be a good idea to get it from the logical expression directly.

Upd: I think we could merge this PR too, we can change it later on with the counter example if there is

alamb · 2024-08-20T17:04:42Z

Ok, I'll plan to merge this PR tomorrow unless anyone else has a different opinion

Thank you all for the discussion. I am glad we got to a good place

alamb · 2024-08-21T15:19:06Z

🚀

alamb · 2024-08-21T15:20:49Z

Marked as API change and updated description to note it removes create_function_physical_name which is public: https://docs.rs/datafusion/latest/datafusion/logical_expr/expr/fn.create_function_physical_name.html

github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate labels Aug 14, 2024

Use schema_name to create the physical_name

60f9ad9

More consistency and less opportunity for column name mismatch.

joroKr21 force-pushed the bugfix/expr-name branch from b71e491 to 60f9ad9 Compare August 14, 2024 14:35

github-actions bot added the physical-expr Physical Expressions label Aug 14, 2024

joroKr21 mentioned this pull request Aug 14, 2024

Standardize the separator in name #10364

Open

joroKr21 marked this pull request as ready for review August 14, 2024 16:51

joroKr21 mentioned this pull request Aug 14, 2024

Use Expr::display_name to create physical_name coralogix/arrow-datafusion#257

Merged

alamb approved these changes Aug 14, 2024

View reviewed changes

viirya reviewed Aug 15, 2024

View reviewed changes

joroKr21 commented Aug 15, 2024

View reviewed changes

viirya reviewed Aug 15, 2024

View reviewed changes

alamb merged commit 1c7209b into apache:main Aug 21, 2024
26 checks passed

alamb added the api change Changes the API exposed to users of the crate label Aug 21, 2024

joroKr21 deleted the bugfix/expr-name branch August 21, 2024 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `schema_name` to create the `physical_name` #11977

Use `schema_name` to create the `physical_name` #11977

joroKr21 commented Aug 14, 2024 •

edited by alamb

Loading

alamb left a comment

alamb Aug 14, 2024

alamb Aug 14, 2024

alamb Aug 14, 2024

jayzhan211 Aug 14, 2024

joroKr21 Aug 15, 2024

jayzhan211 commented Aug 14, 2024 •

edited

Loading

joroKr21 commented Aug 15, 2024

jayzhan211 commented Aug 15, 2024 •

edited

Loading

alamb commented Aug 15, 2024

viirya Aug 15, 2024

viirya commented Aug 15, 2024

viirya commented Aug 15, 2024

joroKr21 Aug 15, 2024

viirya left a comment

alamb commented Aug 19, 2024

viirya commented Aug 19, 2024 •

edited

Loading

jayzhan211 commented Aug 20, 2024 •

edited

Loading

viirya commented Aug 20, 2024

jayzhan211 commented Aug 20, 2024 •

edited

Loading

alamb commented Aug 20, 2024

alamb commented Aug 21, 2024

alamb commented Aug 21, 2024

Use schema_name to create the physical_name #11977

Use schema_name to create the physical_name #11977

Conversation

joroKr21 commented Aug 14, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

alamb Aug 14, 2024

Choose a reason for hiding this comment

alamb Aug 14, 2024

Choose a reason for hiding this comment

alamb Aug 14, 2024

Choose a reason for hiding this comment

jayzhan211 Aug 14, 2024

Choose a reason for hiding this comment

joroKr21 Aug 15, 2024

Choose a reason for hiding this comment

jayzhan211 commented Aug 14, 2024 • edited Loading

joroKr21 commented Aug 15, 2024

jayzhan211 commented Aug 15, 2024 • edited Loading

alamb commented Aug 15, 2024

viirya Aug 15, 2024

Choose a reason for hiding this comment

viirya commented Aug 15, 2024

viirya commented Aug 15, 2024

joroKr21 Aug 15, 2024

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

alamb commented Aug 19, 2024

viirya commented Aug 19, 2024 • edited Loading

jayzhan211 commented Aug 20, 2024 • edited Loading

viirya commented Aug 20, 2024

jayzhan211 commented Aug 20, 2024 • edited Loading

alamb commented Aug 20, 2024

alamb commented Aug 21, 2024

alamb commented Aug 21, 2024

Use `schema_name` to create the `physical_name` #11977

Use `schema_name` to create the `physical_name` #11977

joroKr21 commented Aug 14, 2024 •

edited by alamb

Loading

jayzhan211 commented Aug 14, 2024 •

edited

Loading

jayzhan211 commented Aug 15, 2024 •

edited

Loading

viirya commented Aug 19, 2024 •

edited

Loading

jayzhan211 commented Aug 20, 2024 •

edited

Loading

jayzhan211 commented Aug 20, 2024 •

edited

Loading