alamb commented on code in PR #8113:
URL: https://github.com/apache/arrow-datafusion/pull/8113#discussion_r1390015438
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -352,6 +352,11 @@ impl From<&StringifiedPlan> for protobuf::StringifiedPlan {
PlanType::FinalPhysicalPlan => Some(protobuf::PlanType {
plan_type_enum: Some(FinalPhysicalPlan(EmptyMessage {})),
}),
+ // Make it `todo` to avoid breaking clippy
+ // We do not want to add proto plan type for these two becasue
they are just the same as
+ // InitialPhysicalPlan and FinalPhysicalPlan
+ datafusion_expr::PlanType::InitialPhysicalPlanWithStats
+ | datafusion_expr::PlanType::FinalPhysicalPlanWithStats =>
todo!(),
Review Comment:
I think adding two new types in the proto would be the most consistent with
the existing code
However, as long as this doesn't panic I think it would be ok to merge --
for example, if it returned an internal error.
I don't really understand why there is special serialization for this to be
honest 🤔 maybe we could simplify that too as a follow on PR
##########
datafusion/sqllogictest/test_files/explain.slt:
##########
@@ -245,6 +245,7 @@ logical_plan after eliminate_projection SAME TEXT AS ABOVE
logical_plan after push_down_limit SAME TEXT AS ABOVE
logical_plan TableScan: simple_explain_test projection=[a, b, c]
initial_physical_plan CsvExec: file_groups={1 group:
[[WORKSPACE_ROOT/datafusion/core/tests/data/example.csv]]}, projection=[a, b,
c], has_header=true
+initial_physical_plan_with_stats CsvExec: file_groups={1 group:
[[WORKSPACE_ROOT/datafusion/core/tests/data/example.csv]]}, projection=[a, b,
c], has_header=true, statistics=[Rows=Absent, Bytes=Absent]
Review Comment:
Nice
##########
datafusion/proto/src/logical_plan/to_proto.rs:
##########
@@ -352,6 +352,11 @@ impl From<&StringifiedPlan> for protobuf::StringifiedPlan {
PlanType::FinalPhysicalPlan => Some(protobuf::PlanType {
plan_type_enum: Some(FinalPhysicalPlan(EmptyMessage {})),
}),
+ // Make it `todo` to avoid breaking clippy
+ // We do not want to add proto plan type for these two becasue
they are just the same as
+ // InitialPhysicalPlan and FinalPhysicalPlan
+ datafusion_expr::PlanType::InitialPhysicalPlanWithStats
+ | datafusion_expr::PlanType::FinalPhysicalPlanWithStats =>
todo!(),
Review Comment:
I think adding two new types in the proto would be the most consistent with
the existing code
However, as long as this doesn't panic I think it would be ok to merge --
for example, if it returned an internal error.
I don't really understand why there is special serialization for this to be
honest 🤔 maybe we could simplify that too as a follow on PR
--
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]
