Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move create_physical_expr to phy-expr-common #3 #10188

Closed
wants to merge 18 commits into from

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Apr 23, 2024

Which issue does this PR close?

Closes #10176
Closes #10074
Closes #10144

All in one in this PR.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Apr 23, 2024
@jayzhan211 jayzhan211 marked this pull request as ready for review April 23, 2024 04:09
@alamb
Copy link
Contributor

alamb commented Apr 23, 2024

Hi @jayzhan211 -- if we merge this one, does it close #10176 and #10144 ?

@jayzhan211
Copy link
Contributor Author

Hi @jayzhan211 -- if we merge this one, does it close #10176 and #10144 ?

Yes!

@alamb
Copy link
Contributor

alamb commented Apr 24, 2024

Sorry for being dense -- would you like me to review this PR? Or do you think we should review (and merge) #10176 and #10144 first?

@jayzhan211
Copy link
Contributor Author

jayzhan211 commented Apr 24, 2024

It depends on you and other reviewers, If you don't mind the large PR, definitely it is nice we can merge this one.
Otherwise, we can close #10144 first, and move on to #10176 and finally this one.

I didn't merge #10144 because I'm also curious about what is it like at the final PR, make sure not all the things are moved to common

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this @jayzhan211

I spent some time reviewing this PR this morining

One thing I am missing is the big picture description / story of "what code belongs in what crate". I suspect there is a good story here but I can't quite figure it out.

In this case, I think getting the larger story straight will help avoid churn / code moving back and forth unecessairly. The larger story is also likely the key to understanding the end state.

create_physical_expr

As I understand it, this PR is part of the larger quest to move aggregate functions out of datafusion-physical-expr #8708

In my mind, create_physical_expr is a big switch statement between Expr --> Arc<dyn PhysicalExpr>

Once we have created a Arc<dyn PhysicalExpr> I don't really understand how/why we would ever get an Expr that we need to convert back to Arc<dyn PhysicalExpr> again.

End state of dependencies??

Is the eventual plan to have something like the following? If so, I don't understand why the functions-aggregates needs to depends on create_physical_erxpr

  graph TD;
      functions-aggregates-->physical-expr-common;
      physical-expr-->physical-expr-common;
      core-->functions-aggregates;
      core-->physical-expr;
Loading

@alamb
Copy link
Contributor

alamb commented Apr 25, 2024

Here is another suggestion: #10074 (comment)

@jayzhan211
Copy link
Contributor Author

jayzhan211 commented Apr 25, 2024

Once we have created a Arc I don't really understand how/why we would ever get an Expr that we need to convert back to Arc again.

I agree, but it turns out I didn't find a way to reuse ordering_req in AggregateFunctionExpr, because we can't have physical-expr in AggregateUDFImpl, so is not able to get it straightaway

End state of dependencies??

I agree with the graph, which is in my mind too.

@jayzhan211
Copy link
Contributor Author

If we can move reverse_expr related logic all to logical layer.... 🤔

@jayzhan211 jayzhan211 marked this pull request as draft April 29, 2024 08:16
@jayzhan211 jayzhan211 closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move create_physical_expr to physical-expr-common
2 participants