Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format plan with line number. #9443

Closed
my-vegetable-has-exploded opened this issue Mar 4, 2024 · 8 comments · Fixed by #10019
Closed

Format plan with line number. #9443

my-vegetable-has-exploded opened this issue Mar 4, 2024 · 8 comments · Fixed by #10019
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@my-vegetable-has-exploded
Copy link
Contributor

Is your feature request related to a problem or challenge?

When I try to complete #8416 , I found it hard to review changes in plan in tpch sqllogictests.

I think the main reason is that CoalesceBatchesExec and RepartitionExec::RoundRobinBatch would make git diff aligns the result in unexcepted position.

Takes q11.slt.part as example, it aligns CoalesceBatchesExec and RepartitionExec::RoundRobinBatch but ignore the -- changes.

图片

If we remove CoalesceBatchesExec and RepartitionExec::RoundRobinBatch, it works well to find the -- changes.

图片

图片

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

@my-vegetable-has-exploded my-vegetable-has-exploded added the enhancement New feature or request label Mar 4, 2024
@my-vegetable-has-exploded
Copy link
Contributor Author

I think adding a line number to the front of each line is a easier way.

@alamb
Copy link
Contributor

alamb commented Mar 18, 2024

Adding a line number seems like a reasonable thing to try to me.

The reason we had -- as the prefix was that sqllogictest stripped off leading spaces

@my-vegetable-has-exploded
Copy link
Contributor Author

my-vegetable-has-exploded commented Mar 19, 2024

Maybe we can mark it as a good first issue if needed, and slt tests can regenerate by running cargo test --test sqllogictests -- --include-tpch --complete.

@my-vegetable-has-exploded my-vegetable-has-exploded changed the title Add explain plan without CoalesceBatches and Repartition::RoundRobinBatch for tpch sqllogictests. Format plan with line number. Mar 19, 2024
@alamb alamb added the good first issue Good for newcomers label Mar 19, 2024
@alamb
Copy link
Contributor

alamb commented Mar 19, 2024

Maybe we can mark it as a good first issue if needed, and slt tests can regenerate by running cargo test --test sqllogictests -- --include-tpch --complete.

Good idea @my-vegetable-has-exploded -- I hav done so. Thank you

@duongcongtoai
Copy link
Contributor

Hi, i would love to take this issue as a starting point

@my-vegetable-has-exploded
Copy link
Contributor Author

Hi, i would love to take this issue as a starting point

I think it maybe helpful to take a breif look explain.rs
https://github.com/apache/arrow-datafusion/blob/2f1c3ab679fd553ea9e18067ed894fc6cf1f567a/datafusion/physical-plan/src/explain.rs#L126-L178

@duongcongtoai
Copy link
Contributor

hi @alamb could you help me give one more review to this PR?

@alamb
Copy link
Contributor

alamb commented Apr 15, 2024

Thanks again for #10019 @duongcongtoai -- great stuff. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants