Skip to content

Conversation

irenjj
Copy link
Contributor

@irenjj irenjj commented Mar 15, 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 physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Mar 15, 2025
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.

Thank you so much @irenjj - this looks amazing

I had some small comments / questions, but I also think we can do them as follow on PRs -- this is already better than what is on main

DisplayFormatType::TreeRender => {
if let Some(predicate) = self.predicate() {
writeln!(f, "predicate={predicate}")?;
writeln!(f, "predicate={}", fmt_sql(predicate.as_ref()))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

})
.collect()
};
// TODO: Implement `fmt_sql` for `AggregateFunctionExpr`.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we can make a follow on ticket for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to resolve it, filed #15252.
WindowAggExpr seems have a similar problem🤔.

09)│ -------------------- │
10)│ predicate: │
11)│ string_col@1 != foo │
11)│ string_col != foo
Copy link
Contributor

Choose a reason for hiding this comment

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

so nice

16)│ predicate:
17)│ ticker@1 = A
04)│ date ASC NULLS LAST,
05)│
Copy link
Contributor

Choose a reason for hiding this comment

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

it is somewhat strange to me there is an extra newline. Is that something we can remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an issue I introduced in SortPreservingMergeExec 😭, has been fixed in the latest update.

15)│ FilterExec │
16)│ -------------------- │
17)│ predicate: │
18)│ ticker = A AND CAST(time │
Copy link
Contributor

@alamb alamb Mar 16, 2025

Choose a reason for hiding this comment

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

this looks so nice

giphy

(I am using that image a lot recently 🤔 )

irenjj and others added 2 commits March 16, 2025 18:48
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@irenjj
Copy link
Contributor Author

irenjj commented Mar 16, 2025

Thanks @alamb for your review.

@alamb alamb merged commit d23c52f into apache:main Mar 16, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 16, 2025

Thanks again @irenjj -- merging this one in to unblock the next in the sequence

@irenjj irenjj deleted the irenjj/expr_in_tree branch March 16, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify the printing of all plans containing expr in tree mode.

2 participants