You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "qrilka (via GitHub)" <gi...@apache.org> on 2023/06/17 16:09:58 UTC
[GitHub] [arrow-datafusion] qrilka opened a new pull request, #6711: Display all partitions and files in EXPLAIN VERBOSE
qrilka opened a new pull request, #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711
Adds DisplayAs trait for structs which could show more details when formatted in the verbose mode
Resolves https://github.com/apache/arrow-datafusion/issues/6383
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #6383.
<!-- # Rationale for this change -->
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Notes about the implementation:
* new trait `physical_plan::display::DisplayAs` was created similar to `physical_plan::ExecutionPlan::fmt_as` (but this method is left in its current trait)
* `verbose` mode wasn't actually used in the code and to make it work I added `verbose` as an argument of `physical_plan::display::DisplayableExecutionPlan::indent` and moved `to_stringified` into that trait and also added `verbose` into this method
* as a result new we have `fmt_as` in two different traits and also 2 slightly different `to_stringified` in two traits as well but I couldn't find a good way around that
* I've tested the implementation manually as it's described in https://github.com/apache/arrow-datafusion/issues/6383#issuecomment-1554300787 but I wonder if there is some way to add something similar to the code base. For example my first implementation failed this manual test with unit tests succeding (just because `verbose` wasn't actually used)
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
New unit tests were added. Plus manual test from the ticket also passes
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
The docs don't have much details about `EXPLAIN` so I'm not sure that we need to add this minor detail.
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#discussion_r1234109250
##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -30,6 +30,8 @@ use super::{accept, ExecutionPlan, ExecutionPlanVisitor};
pub enum DisplayFormatType {
/// Default, compact format. Example: `FilterExec: c12 < 10.0`
Default,
+ /// Verbose, showing all available details
Review Comment:
👍
##########
datafusion/substrait/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -66,8 +66,11 @@ async fn parquet_exec() -> Result<()> {
consumer::from_substrait_rel(&mut ctx, substrait_rel.as_ref(), &HashMap::new())
.await?;
- let expected = format!("{}", displayable(parquet_exec.as_ref()).indent());
- let actual = format!("{}", displayable(parquet_exec_roundtrip.as_ref()).indent());
+ let expected = format!("{}", displayable(parquet_exec.as_ref()).indent(true));
Review Comment:
💯 for including verbose
##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -192,11 +209,29 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
}
}
-impl<'a> ToStringifiedPlan for DisplayableExecutionPlan<'a> {
- fn to_stringified(
- &self,
- plan_type: crate::logical_expr::PlanType,
- ) -> StringifiedPlan {
- StringifiedPlan::new(plan_type, self.indent().to_string())
+/// Trait for types which could have additional details when formatted in `Verbose` mode
Review Comment:
🤔 maybe eventually we can combine this into `ExecutionPlan` so that everything that implemented `ExecutionPlan` would also need to implement `DisplayAs`.
That would reduce the duplication across the traits, howe er, it would be an API change and result in non trivial code churn so I think we can consider it as part of a follow on PR
```
trait ExecutionPlan: DisplayAs {
...
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#discussion_r1234163580
##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -192,11 +209,29 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
}
}
-impl<'a> ToStringifiedPlan for DisplayableExecutionPlan<'a> {
- fn to_stringified(
- &self,
- plan_type: crate::logical_expr::PlanType,
- ) -> StringifiedPlan {
- StringifiedPlan::new(plan_type, self.indent().to_string())
+/// Trait for types which could have additional details when formatted in `Verbose` mode
Review Comment:
> Sure but why do you think it won't be trivial? If https://github.com/rust-lang/rust/issues/31844 stabilized it would be much easier of course but as far as I see, the change is tedious but should be straitforward: just split fmt_as into as separate impl. Do I miss anything here?
I agree the change would be mechanical (to move `fmt_as` into a separate `impl`), but it would be required for all `impl ExecutionPlan` impls which is why I would I not call it "trivial" -- but i agree that is just my opinion and how others might have a different opinion
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#issuecomment-1599358334
Thanks again @qrilka
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] qrilka commented on a diff in pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "qrilka (via GitHub)" <gi...@apache.org>.
qrilka commented on code in PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#discussion_r1234151691
##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -192,11 +209,29 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
}
}
-impl<'a> ToStringifiedPlan for DisplayableExecutionPlan<'a> {
- fn to_stringified(
- &self,
- plan_type: crate::logical_expr::PlanType,
- ) -> StringifiedPlan {
- StringifiedPlan::new(plan_type, self.indent().to_string())
+/// Trait for types which could have additional details when formatted in `Verbose` mode
Review Comment:
Sure but why do you think it won't be trivial? If https://github.com/rust-lang/rust/issues/31844 stabilized it would be much easier of course but as far as I see, the change is tedious but should be straitforward: just split `fmt_as` into as separate `impl`. Do I miss anything 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#issuecomment-1599581770
> Sure, I guess it makes sense to create a follow-up PR with trait ExecutionPlan: DisplayAs, right?
Yes, this is what I think makes the most sense
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] qrilka commented on pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "qrilka (via GitHub)" <gi...@apache.org>.
qrilka commented on PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#issuecomment-1599452436
Sure, I guess it makes sense to create a follow-up PR with `trait ExecutionPlan: DisplayAs`, right?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#issuecomment-1599305042
I merged up from main and plan to merge this PR once it passes the tests
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6711: Display all partitions and files in EXPLAIN VERBOSE
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6711:
URL: https://github.com/apache/arrow-datafusion/pull/6711#discussion_r1234163580
##########
datafusion/core/src/physical_plan/display.rs:
##########
@@ -192,11 +209,29 @@ impl<'a, 'b> ExecutionPlanVisitor for IndentVisitor<'a, 'b> {
}
}
-impl<'a> ToStringifiedPlan for DisplayableExecutionPlan<'a> {
- fn to_stringified(
- &self,
- plan_type: crate::logical_expr::PlanType,
- ) -> StringifiedPlan {
- StringifiedPlan::new(plan_type, self.indent().to_string())
+/// Trait for types which could have additional details when formatted in `Verbose` mode
Review Comment:
> Sure but why do you think it won't be trivial? If https://github.com/rust-lang/rust/issues/31844 stabilized it would be much easier of course but as far as I see, the change is tedious but should be straitforward: just split fmt_as into as separate impl. Do I miss anything here?
I agree the change would be mechanical (to move `fmt_as` into a separate `impl`), but it would be required for all `impl ExecutionPlan` impls which is why I would I not call it "trivial" -- but i agree that is just my opinion and how others might have a different opinon
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org