You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "irenjj (via GitHub)" <gi...@apache.org> on 2025/02/15 03:54:04 UTC
[PR] feat: pretty explain [datafusion]
irenjj opened a new pull request, #14677:
URL: https://github.com/apache/datafusion/pull/14677
## 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.
-->
- Partof #9371
## 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.
-->
## 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)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1969310940
##########
datafusion/datasource/src/source.rs:
##########
@@ -43,6 +44,7 @@ pub trait DataSource: Send + Sync {
) -> datafusion_common::Result<SendableRecordBatchStream>;
fn as_any(&self) -> &dyn Any;
fn fmt_as(&self, t: DisplayFormatType, f: &mut Formatter) -> fmt::Result;
+ fn collect_info(&self) -> HashMap<String, String>;
Review Comment:
Why do we need a new interface to collect info for pretty explain? Looks like it's possible to implement inside `fmt_as()`.
DuckDB explains also have many non key-value patterns like the following example, we will want the formater to be more flexible.
```
┌─────────────┴─────────────┐
│ READ_PARQUET │
│ ──────────────────── │
│ Function: │
│ READ_PARQUET │
│ │
│ Projections: │
│ l_orderkey │
│ l_partkey │
│ l_suppkey │
│ l_linenumber │
│ l_quantity │
│ l_extendedprice │
│ │
│ ~6046496 Rows │
└───────────────────────────┘
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "irenjj (via GitHub)" <gi...@apache.org>.
irenjj commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2677245452
> Happy to see this happen.
>
> Maybe it's better to open a tracking issue to record next todos, such as rich other physical operator's information to display. Then we can make our new explain perfect in parallel after the PR is merged.
I will organize it later today.❤️
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2660969350
It's my lovest todo:
<img width="549" alt="image" src="https://github.com/user-attachments/assets/4aa3fb86-8382-4eea-8e64-e32822ef9bcb" />
Thank you for making it happen!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "irenjj (via GitHub)" <gi...@apache.org>.
irenjj commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1971677974
##########
datafusion/physical-plan/src/render_tree.rs:
##########
@@ -0,0 +1,158 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::cmp;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+use crate::ExecutionPlan;
+
+#[allow(dead_code)]
Review Comment:
It will warn while cargo build:
```
warning: fields `x` and `y` are never read
--> datafusion/physical-plan/src/render_tree.rs:29:9
|
28 | pub struct Coordinate {
| ---------- fields in this struct
29 | pub x: usize,
| ^
30 | pub y: usize,
| ^
|
= note: `#[warn(dead_code)]` on by default
```
the `Coordinate` is inspired from duckdb's implementation, I'll figure out if it's necessary to introduce such a structure.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970419771
##########
datafusion/sqllogictest/test_files/explain_tree.slt:
##########
@@ -0,0 +1,198 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Tests for tree explain
+
+
+
+statement ok
+set datafusion.explain.format = "pretty";
+
+######## Setup Data Files #######
+
+# table1: CSV
+query I
+COPY (VALUES (1, 'foo', 1, '2023-01-01'), (2, 'bar', 2, '2023-01-02'), (3, 'baz', 3, '2023-01-03'))
+TO 'test_files/scratch/explain_tree/table1.csv';
+----
+3
+
+statement ok
+CREATE EXTERNAL TABLE table1 (
+ int_col INT,
+ string_col TEXT,
+ bigint_col BIGINT,
+ date_col DATE
+)
+STORED AS CSV
+LOCATION 'test_files/scratch/explain_tree/table1.csv';
+
+# table2: Parquet
+query I
+COPY (SELECT * from table1)
+TO 'test_files/scratch/explain_tree/table2.parquet'
+----
+3
+
+statement ok
+CREATE EXTERNAL TABLE table2
+STORED AS PARQUET
+LOCATION 'test_files/scratch/explain_tree/table2.parquet';
+
+
+# table3: Memoru
+statement ok
+CREATE TABLE table3 as select * from table1;
+
+######## Begin Queries ########
+
+# Filter
+query TT
+explain SELECT int_col FROM table1 WHERE string_col != 'foo';
+----
+logical_plan
+01)Projection: table1.int_col
+02)--Filter: table1.string_col != Utf8("foo")
+03)----TableScan: table1 projection=[int_col, string_col], partial_filters=[table1.string_col != Utf8("foo")]
+physical_plan
+01)┌───────────────────────────┐
+02)│ CoalesceBatchesExec │
+03)└─────────────┬─────────────┘
+04)┌─────────────┴─────────────┐
+05)│ FilterExec │
+06)└─────────────┬─────────────┘
+07)┌─────────────┴─────────────┐
+08)│ RepartitionExec │
+09)└─────────────┬─────────────┘
+10)┌─────────────┴─────────────┐
+11)│ DataSourceExec │
+12)└───────────────────────────┘
+
+# Aggregate
+query TT
+explain SELECT string_col, SUM(bigint_col) FROM table1 GROUP BY string_col;
+----
+logical_plan
+01)Aggregate: groupBy=[[table1.string_col]], aggr=[[sum(table1.bigint_col)]]
+02)--TableScan: table1 projection=[string_col, bigint_col]
+physical_plan
+01)┌───────────────────────────┐
+02)│ AggregateExec │
+03)└─────────────┬─────────────┘
+04)┌─────────────┴─────────────┐
+05)│ CoalesceBatchesExec │
+06)└─────────────┬─────────────┘
+07)┌─────────────┴─────────────┐
+08)│ RepartitionExec │
+09)└─────────────┬─────────────┘
+10)┌─────────────┴─────────────┐
+11)│ AggregateExec │
+12)└─────────────┬─────────────┘
+13)┌─────────────┴─────────────┐
+14)│ RepartitionExec │
+15)└─────────────┬─────────────┘
+16)┌─────────────┴─────────────┐
+17)│ DataSourceExec │
+18)└───────────────────────────┘
+
+# 2 Joins
+query TT
+explain SELECT table1.string_col, table2.date_col FROM table1 JOIN table2 ON table1.int_col = table2.int_col;
+----
+logical_plan
+01)Projection: table1.string_col, table2.date_col
+02)--Inner Join: table1.int_col = table2.int_col
+03)----TableScan: table1 projection=[int_col, string_col]
+04)----TableScan: table2 projection=[int_col, date_col]
+physical_plan
+01)┌───────────────────────────┐
+02)│ CoalesceBatchesExec │
+03)└─────────────┬─────────────┘
+04)┌─────────────┴─────────────┐
+05)│ HashJoinExec ├──────────────┐
+06)└─────────────┬─────────────┘ │
+07)┌─────────────┴─────────────┐┌─────────────┴─────────────┐
+08)│ CoalesceBatchesExec ││ CoalesceBatchesExec │
+09)└─────────────┬─────────────┘└─────────────┬─────────────┘
+10)┌─────────────┴─────────────┐┌─────────────┴─────────────┐
+11)│ RepartitionExec ││ RepartitionExec │
+12)└─────────────┬─────────────┘└─────────────┬─────────────┘
+13)┌─────────────┴─────────────┐┌─────────────┴─────────────┐
+14)│ RepartitionExec ││ RepartitionExec │
+15)└─────────────┬─────────────┘└─────────────┬─────────────┘
+16)┌─────────────┴─────────────┐┌─────────────┴─────────────┐
+17)│ DataSourceExec ││ DataSourceExec │
+18)└───────────────────────────┘└───────────────────────────┘
+
+# 3 Joins
+query TT
+explain SELECT
+ table1.string_col,
+ table2.date_col,
+ table3.date_col
+FROM
+ table1 JOIN table2 ON table1.int_col = table2.int_col
+ JOIN table3 ON table2.int_col = table3.int_col;
+----
+logical_plan
Review Comment:
It is pretty neat to see the joins visually like this
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "irenjj (via GitHub)" <gi...@apache.org>.
irenjj commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970734646
##########
datafusion/physical-plan/src/memory.rs:
##########
@@ -192,6 +192,7 @@ impl DisplayAs for LazyMemoryExec {
.join(", ")
)
}
+ _ => todo!(),
Review Comment:
That's because previously the information for tree_render was obtained from collect_info, and later when it was changed to be fetched from fmt_as, this TODO would be replaced.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970404144
##########
datafusion/sqllogictest/test_files/explain_tree.slt:
##########
@@ -0,0 +1,186 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
I also took the liberty of adding a .slt based test 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970399198
##########
datafusion/physical-plan/src/render_tree.rs:
##########
@@ -0,0 +1,158 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::cmp;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+use crate::ExecutionPlan;
+
+#[allow(dead_code)]
Review Comment:
Is it still deadcode? it doesn't look like it
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `tree` / pretty explain mode [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2697397568
🚀 thank you again so much @irenjj -- this is so cool. A great step towards better explain plans.
I am working on enhancing the follow on ticket you filed in https://github.com/apache/datafusion/issues/14914 ❤️
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970395626
##########
datafusion/physical-plan/src/memory.rs:
##########
@@ -192,6 +192,7 @@ impl DisplayAs for LazyMemoryExec {
.join(", ")
)
}
+ _ => todo!(),
Review Comment:
Can you return Error instead of todo?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `tree` / pretty explain mode [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2695592755
This is next on my list of things to merge
What i would like ot do is merge this PR in and then file follow on subtasks in
- https://github.com/apache/datafusion/issues/14914
I am sorry I have been slammed trying to get the DataFusion 46 release out
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2660854503
😍
I have been dreaming about this @irenjj -- can't wait to check it out
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970453979
##########
datafusion/physical-plan/src/display.rs:
##########
@@ -448,13 +492,386 @@ impl ExecutionPlanVisitor for GraphvizVisitor<'_, '_> {
}
}
+struct TreeRenderVisitor<'a, 'b> {
Review Comment:
This looks like it is substantially based on the duckdb implementation: https://github.com/duckdb/duckdb/pull/1046/files#diff-d1323eac8a19eb719b259e964df5e9248fb17bf3872f4ea0776566dd2a62ab4b
I think we should at least credit where it came from
I also wonder if we should use the latest version:
https://github.com/duckdb/duckdb/blob/main/src/common/tree_renderer/text_tree_renderer.cpp
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2688949532
I hope to look at this later today
Looks like the follow on ticket is filed.
- https://github.com/apache/datafusion/issues/14914
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `tree` / pretty explain mode [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14677:
URL: https://github.com/apache/datafusion/pull/14677
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "irenjj (via GitHub)" <gi...@apache.org>.
irenjj commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2676863874
PTAL @alamb @xudong963 👀
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2683120383
Interestingly, the CI failure
- https://github.com/apache/datafusion/actions/runs/13529388760/job/37808013086?pr=14677
Is due to the fact that order of the hash map output is non deterministic. I will push a fix
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970418321
##########
datafusion/physical-plan/src/memory.rs:
##########
@@ -192,6 +192,7 @@ impl DisplayAs for LazyMemoryExec {
.join(", ")
)
}
+ _ => todo!(),
Review Comment:
I think following the other APIs (aka return the same as Indent) would be better still
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: `tree` / pretty explain [datafusion]
Posted by "irenjj (via GitHub)" <gi...@apache.org>.
irenjj commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1971477800
##########
datafusion/physical-plan/src/display.rs:
##########
@@ -448,13 +492,386 @@ impl ExecutionPlanVisitor for GraphvizVisitor<'_, '_> {
}
}
+struct TreeRenderVisitor<'a, 'b> {
Review Comment:
Credited! and render_tree.rs is based on duckdb implementation too.
It's latest version when I'm implementing(last week).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#issuecomment-2678592939
Looks awesome -- I have put this on my review queue. So exciting
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970377001
##########
datafusion/common/src/config.rs:
##########
@@ -704,6 +704,10 @@ config_namespace! {
/// When set to true, the explain statement will print schema information
pub show_schema: bool, default = false
+
+ /// Display format of explain. Default is "indent".
+ /// When set to "pretty", it will print the plan in a tree-rendered format.
Review Comment:
rather than `"pretty"` that prints out a tree I suggest we change the option name to `"tree"`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970400027
##########
datafusion/physical-plan/src/render_tree.rs:
##########
@@ -0,0 +1,158 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::cmp;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+use crate::ExecutionPlan;
+
+#[allow(dead_code)]
Review Comment:
It would be great to add some comments in this module too
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: pretty explain [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14677:
URL: https://github.com/apache/datafusion/pull/14677#discussion_r1970408575
##########
datafusion/datasource/src/source.rs:
##########
@@ -43,6 +44,7 @@ pub trait DataSource: Send + Sync {
) -> datafusion_common::Result<SendableRecordBatchStream>;
fn as_any(&self) -> &dyn Any;
fn fmt_as(&self, t: DisplayFormatType, f: &mut Formatter) -> fmt::Result;
+ fn collect_info(&self) -> HashMap<String, String>;
Review Comment:
I agree with this -- it would be much better to avoid a new interface and instead update the existing `fmt_as` API
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org