gene-bordegaray commented on code in PR #23030:
URL: https://github.com/apache/datafusion/pull/23030#discussion_r3448151841
##########
datafusion/proto-models/proto/datafusion.proto:
##########
@@ -148,9 +148,19 @@ message RepartitionNode {
oneof partition_method {
uint64 round_robin = 2;
HashRepartition hash = 3;
+ RangeRepartition range = 4;
}
}
+message RangeSplitPoint {
+ repeated datafusion_common.ScalarValue value = 1;
+}
+
+message RangeRepartition {
+ repeated SortExprNode expr = 1;
+ repeated RangeSplitPoint split_points = 2;
Review Comment:
I also think this should just be `split_point`
##########
datafusion/proto-models/proto/datafusion.proto:
##########
@@ -148,9 +148,19 @@ message RepartitionNode {
oneof partition_method {
uint64 round_robin = 2;
HashRepartition hash = 3;
+ RangeRepartition range = 4;
}
}
+message RangeSplitPoint {
+ repeated datafusion_common.ScalarValue value = 1;
+}
+
+message RangeRepartition {
+ repeated SortExprNode expr = 1;
Review Comment:
yes, was going to cmment this 😄 thank you
##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -899,3 +899,14 @@ fn parse_required_expr(
fn proto_error<S: Into<String>>(message: S) -> Error {
Error::General(message.into())
}
+
+pub fn parse_protobuf_range_split_point(
Review Comment:
could this and `serialize_range_split_point` be `pub(super)`
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -3509,3 +3331,33 @@ async fn roundtrip_join_null_equality() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn roundtrip_range_partitioning() -> Result<()> {
Review Comment:
yes I think this would be good maybe a set like:
1. single col / single split-point
2. multi col / multi split-points
3. Non-default sort and null options
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -3509,3 +3509,33 @@ async fn roundtrip_join_null_equality() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn roundtrip_range_partitioning() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let schema = Schema::new(vec![
+ Field::new("a", DataType::Int64, true),
+ Field::new("b", DataType::Decimal128(15, 2), true),
+ ]);
+
+ ctx.register_csv(
Review Comment:
nit: we cuold just do an in memory or empty table tests here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
