reuse datafusion physical planner in ballista building from protobuf

[jimexist]

Which issue does this PR close?

Closes #.

Rationale for this change

reuse datafusion physical planner in ballista building from protobuf.

so far there are duplicated code within ballista and datafusion planner because the latter needs to handle alias but for ballista the expr name is flattened in protobuf, resulting two similar codepath in building the aggregation exec.

this tries to unify both and reuse code.

What changes are included in this PR?

Are there any user-facing changes?

[codecov-commenter]

Codecov Report

Merging #532 (d0f2804) into master (8f84564) will increase coverage by 0.12%.
The diff coverage is 66.21%.

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   76.04%   76.17%   +0.12%     
==========================================
  Files         157      156       -1     
  Lines       27098    27017      -81     
==========================================
- Hits        20608    20580      -28     
+ Misses       6490     6437      -53     

Impacted Files	Coverage Δ
...ta/rust/core/src/serde/physical_plan/from_proto.rs	39.83% <47.05%> (+2.11%)	⬆️
datafusion/src/physical_plan/planner.rs	78.79% <61.90%> (-1.41%)	⬇️
datafusion/src/physical_plan/windows.rs	86.46% <100.00%> (+0.24%)	⬆️
...sta/rust/core/src/serde/logical_plan/from_proto.rs	35.04% <0.00%> (-0.17%)	⬇️
datafusion/src/physical_plan/window_frames.rs
datafusion/src/sql/planner.rs	84.85% <0.00%> (+0.23%)	⬆️
datafusion/src/logical_plan/window_frames.rs	89.39% <0.00%> (+2.78%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f84564...d0f2804. Read the comment docs.

[alamb]

Looks like a nice change to me -- but I think someone more familiar with Ballista to review it too -- @andygrove / @edrevo can you suggest someone?

[andygrove]

I will have time tomorrow to review this and other pending ballista PRs

…

On Thu, Jun 10, 2021, 9:47 AM QP Hou ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#532 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHEBRANXOWJAIYJNOMKQTTTSDNCVANCNFSM46NLUHWQ> .

[edrevo]

nit: once this is merged, it would be cool to add a link to this code in the respective issues that are tracking these missing features to make it easier for new contributors to start contributing.

[edrevo]

I just realized that this PR changes the behavior when there is a non-empty partition_by/order_by/window_frame: before (at least in ballista) it would error, whereas now it is silently ignored. Maybe it is worth erroring if they aren't empty to make it explicit that there is no support?

[jimexist]

it'll be further developed in #520

[jimexist]

but that's a good point let me add guard here.

[jimexist]

this is fixed now

[andygrove]

LGTM. I pulled the branch locally and ran the integration tests.