feat: logical plan protobuf representation for range repartitioning#23030
Conversation
| Error::General(message.into()) | ||
| } | ||
|
|
||
| pub fn parse_protobuf_range_split_point( |
There was a problem hiding this comment.
Copied this & serialize_range_split_point from the physical plan logic by @gene-bordegaray ! 😁
Merge new MergeInto/DML imports from upstream with range partitioning additions.
|
@gene-bordegaray tagging you in here - please let me know your thoughts! Looking forward to any feedback & working together further on this effort! 😁 🎉 |
gene-bordegaray
left a comment
There was a problem hiding this comment.
overall looking good 😄 , thank you for this work. Left some minor comments 👍
|
@gene-bordegaray pushed changes to address your comments - let me know your thoughts! Really appreciate the time taken to review! |
gene-bordegaray
left a comment
There was a problem hiding this comment.
This looks good to me now 👍
Thank you!
For next reviewer, there is some duplication in the helpers parse_protobuf_range_split_point / serialize_range_split_point across logical and physical but think it is ok for now. If another use pops up we could make some common helper for this.
cc: @gabotechs
alamb
left a comment
There was a problem hiding this comment.
Thank you @saadtajwar and @gene-bordegaray -- looks good to me
| Field::new("ts", DataType::Int64, false), | ||
| Field::new("region", DataType::Utf8, false), | ||
| ])); | ||
| let table = Arc::new(datafusion::datasource::empty::EmptyTable::new(Arc::clone( |
There was a problem hiding this comment.
as a minor nit, datafusion::datasource::empty::EmptyTable: is pretty hard on the eyes. It would be nicer if there was a single use datafusion::datasource::empty::EmptyTable; at the top and then used down here (and the other tests)
There was a problem hiding this comment.
Done! Thank you! Is there anything needed on my end to get this in? Appreciate you taking the time to review this!
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
@Dandandan & @alamb & @gene-bordegaray - thanks so much again for helping get this in team! Super excited to continue contributing to Datafusion & please let me know if there's anything else on this specific initiative I can help out with! 😁 🎉 |
No problem, I should be thanking you for great work. I have other related issues up for range partitioning. Feel free to pick up any you want to take on. I am more than happy to review and answer questions |
Which issue does this PR close?
Rationale for this change
The range repartitioning scheme for logical plans does not currently have a protobuf representation.
What changes are included in this PR?
A protobuf representation of the
RangeRepartitionstruct was added todatafusion.proto, and the codegened Rust types were created. Added logic for serializing and deserializing to and from the protobuf representation, and a roundtrip test as well!Are these changes tested?
Yes! Added a test in
roundtrip_logical_planAre there any user-facing changes?
No, adding internal protobuf serialization support for an existing logical plan variant