Skip to content

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
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

// TODO: collect info
write!(f, "")
let common_prefix_length = self.common_prefix_length;
match self.fetch {
Copy link
Contributor

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

/// 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          │
└───────────────────────────┘

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

DisplayFormatType::TreeRender => match self.fetch {
Some(fetch) => {
writeln!(f, "limit={fetch}")?;
writeln!(f, "sort keys=[{}]", self.expr)
Copy link
Contributor

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

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement tree explain for PartialSortExec

2 participants