Skip to content

Move physical plan serde from Ballista to DataFusion #4390

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Nov 30, 2022
Merged

Move physical plan serde from Ballista to DataFusion #4390

merged 10 commits into from
Nov 30, 2022

Conversation

Kikkon
Copy link
Contributor

@Kikkon Kikkon commented Nov 27, 2022

Signed-off-by: kikkon [email protected]

Which issue does this PR close?

Closes #3949

Rationale for this change

What changes are included in this PR?

Move physical plan serde from Ballista to DataFusion

Are these changes tested?

Move the roundtrip_tests in Ballista for verifying the correctness of serde

Are there any user-facing changes?

no

@Kikkon Kikkon force-pushed the kikkon/physical_proto branch from bc4c759 to 13feaa6 Compare November 27, 2022 13:32
@Kikkon Kikkon marked this pull request as ready for review November 27, 2022 15:09
@andygrove
Copy link
Member

Thanks @Kikkon. This is looking great. I see that this moves serde of physical expressions to DataFusion, but not physical operators. Do you plan to address those in a future PR?

@Kikkon Kikkon force-pushed the kikkon/physical_proto branch from e2e9674 to 80174e3 Compare November 29, 2022 07:37
@Kikkon
Copy link
Contributor Author

Kikkon commented Nov 29, 2022

Thanks @Kikkon. This is looking great. I see that this moves serde of physical expressions to DataFusion, but not physical operators. Do you plan to address those in a future PR?

Sounds good, I'll create a new issue to track this.

@Kikkon Kikkon mentioned this pull request Nov 29, 2022
Closed
alamb
alamb approved these changes Nov 29, 2022
View reviewed changes
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.

Thanks @Kikkon -- I think this looks great. There are a few comments from @andygrove and myself but I don't think they are needed prior to merging this PR. I think it would be fine to fill out this functionality as it gets used.

Thanks again

datafusion/proto/src/physical_plan/mod.rs
}
})
.collect::<Result<Vec<_>, _>>()?;
//todo fill partition keys and sort keys
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still a todo? Perhaps we can file a follow on ticket

andygrove
andygrove approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Thanks @Kikkon. LGTM.

@andygrove
Copy link
Member

@Kikkon Could you fix the merge conflict?

@Kikkon Kikkon force-pushed the kikkon/physical_proto branch from 79f271f to f7f4b2c Compare November 30, 2022 15:48
@Kikkon
Copy link
Contributor Author

Kikkon commented Nov 30, 2022

@andygrove @alamb Thanks for your review, now it looks like it's ready to be merged 😇

alamb
alamb approved these changes Nov 30, 2022
View reviewed changes
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.

Let's do it! Thank you @Kikkon

@alamb alamb merged commit f2e2c29 into apache:master Nov 30, 2022
@ursabot
Copy link

ursabot commented Nov 30, 2022

Benchmark runs are scheduled for baseline = 23b4495 and contender = f2e2c29. f2e2c29 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@Kikkon Kikkon deleted the kikkon/physical_proto branch December 1, 2022 11:28
@mingmwang
Copy link
Contributor

mingmwang commented Dec 6, 2022 •
edited
Loading

Looks like there are other Ballista specific structs were added to DataFusion. Was that expected ?

@andygrove @alamb @Kikkon

https://github.com/apache/arrow-datafusion/blob/740a4fa2c6ba4b85875a433bb86e5b00435a5969/datafusion/proto/proto/datafusion.proto#L1296-L1367

@alamb
Copy link
Contributor

alamb commented Dec 6, 2022

Looks like there are other Ballista specific structs were added to DataFusion. Was that expected ?

It was not expected @mingmwang -- @Kikkon would you be willing to make a PR to remove them?

@yahoNanJing yahoNanJing mentioned this pull request Dec 16, 2022
Closed
@yahoNanJing yahoNanJing mentioned this pull request Dec 16, 2022
Merged
@nvartolomei nvartolomei mentioned this pull request Dec 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@andygrove andygrove andygrove approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move physical plan serde from Ballista to DataFusion
5 participants
@Kikkon @andygrove @ursabot @mingmwang @alamb