Skip to content

Comments

Implement tree explain for PartialSortExec#15066

Merged
alamb merged 6 commits intoapache:mainfrom
irenjj:explain_PartialSortExec
Mar 9, 2025
Merged

Implement tree explain for PartialSortExec#15066
alamb merged 6 commits intoapache:mainfrom
irenjj:explain_PartialSortExec

Conversation

@irenjj
Copy link
Contributor

@irenjj irenjj commented Mar 7, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 7, 2025
@irenjj irenjj marked this pull request as ready for review March 7, 2025 15:31
alamb
alamb reviewed Mar 7, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @irenjj -- this looks great. I think we could make it simpler though . Let me know what you think

datafusion/physical-plan/src/sorts/partial_sort.rs Outdated
// TODO: collect info
write!(f, "")
let common_prefix_length = self.common_prefix_length;
match self.fetch {
Copy link
Contributor

@alamb alamb Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend we make this simpler -- specifically the common_prefix_length is too detailed for the TreeRender per

datafusion/datafusion/physical-plan/src/display.rs

Lines 48 to 75 in 2458693

/// TreeRender, displayed in the `tree` explain type.
///
/// This format is inspired by DuckDB's explain plans. The information
/// presented should be "user friendly", and contain only the most relevant
/// information for understanding a plan. It should NOT contain the same level
/// of detail information as the [`Self::Default`] format.
///
/// In this mode, each line contains a key=value pair.
/// Everything before the first `=` is treated as the key, and everything after the
/// first `=` is treated as the value.
///
/// For example, if the output of `TreeRender` is this:
/// ```text
/// partition_sizes=[1]
/// partitions=1
/// ```
///
/// It is rendered in the center of a box in the following way:
///
/// ```text
/// ┌───────────────────────────┐
/// │ DataSourceExec │
/// │ -------------------- │
/// │ partition_sizes: [1] │
/// │ partitions: 1 │
/// └───────────────────────────┘
/// ```
TreeRender,

I also think the TopK / Fetch thing is confusing

What I would recommend is:

01)┌───────────────────────────┐
02)│      PartialSortExec      │
03)│    --------------------   │
07)│           sort:           │
08)│[a@1 ASC NULLS LAST, b@2...│
09)└─────────────┬─────────────┘
10)┌─────────────┴─────────────┐
11)│     StreamingTableExec    │
12)└───────────────────────────┘

And

01)┌───────────────────────────┐
02)│      PartialSortExec      │
03)│    --------------------   │
04)│       fetch=50      │
09)│           sort:           │
10)│[a@1 ASC NULLS LAST, b@2...│
11)└─────────────┬─────────────┘

Here is what duckdb does for refernece:

D create table foo (x int);
D insert into foo values (1);
D explain select * from foo order by x;

┌─────────────────────────────┐
│┌───────────────────────────┐│
││       Physical Plan       ││
│└───────────────────────────┘│
└─────────────────────────────┘
┌───────────────────────────┐
│         PROJECTION        │
│    ────────────────────   │
│__internal_decompress_integ│
│     ral_integer(#0, 1)    │
│                           │
│          ~1 Rows          │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│          ORDER_BY         │
│    ────────────────────   │
│   memory.main.foo.x ASC   │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│         PROJECTION        │
│    ────────────────────   │
│__internal_compress_integra│
│     l_utinyint(#0, 1)     │
│                           │
│          ~1 Rows          │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│         SEQ_SCAN          │
│    ────────────────────   │
│         Table: foo        │
│   Type: Sequential Scan   │
│       Projections: x      │
│                           │
│          ~1 Rows          │
└───────────────────────────┘

alamb
alamb approved these changes Mar 8, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @irenjj

datafusion/physical-plan/src/sorts/partial_sort.rs Outdated
DisplayFormatType::TreeRender => match self.fetch {
Some(fetch) => {
writeln!(f, "limit={fetch}")?;
writeln!(f, "sort keys=[{}]", self.expr)
Copy link
Contributor

@alamb alamb Mar 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend avoiding the [ and ] if possible

@irenjj irenjj force-pushed the explain_PartialSortExec branch from 5816313 to 2006ebb Compare March 8, 2025 13:20
@alamb
Copy link
Contributor

alamb commented Mar 9, 2025

Thanks again @irenjj

@alamb alamb merged commit 618880e into apache:main Mar 9, 2025
24 checks passed
@irenjj irenjj deleted the explain_PartialSortExec branch March 13, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement tree explain for PartialSortExec

2 participants

@irenjj @alamb