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

Implement Debug for `SessionStateBuilder #12555

Closed
Tracked by #12550
alamb opened this issue Sep 20, 2024 · 10 comments
Closed
Tracked by #12550

Implement Debug for `SessionStateBuilder #12555

alamb opened this issue Sep 20, 2024 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Sep 20, 2024

Is your feature request related to a problem or challenge?

Part of #12550

While working on https://github.com/datafusion-contrib/datafusion-dft I found it would be nice to have Debug for SessionStateBuilder so I could debug the actual state

Describe the solution you'd like

I would like SessionStateBuilder to implement Debug

Describe alternatives you've considered

I started working on this and it turns out many of the sub structs (like Analyzer and Optimizer don't implement debug). This I recommend making a series of PRs to slowly add debug to the necessary structures so we can enable it for evertthing

Additional context

No response

@alamb
Copy link
Contributor Author

alamb commented Sep 20, 2024

A good way to find out what is left would be to put #[derive(Debug)] on SessionStateBuilder and then work down the errors

@alamb alamb added the good first issue Good for newcomers label Sep 20, 2024
@alamb
Copy link
Contributor Author

alamb commented Sep 20, 2024

I think this is also a good first issue as it has a well defined scope and it is largely mechanical (doesn't require deep codebase knowledge)

Note I made a first step here: #12556

@alamb
Copy link
Contributor Author

alamb commented Sep 20, 2024

Here is another PR: #12557

@OussamaSaoudi
Copy link
Contributor

take

@AnthonyZhOon
Copy link
Contributor

Added a PR: #12624

@AnthonyZhOon
Copy link
Contributor

I started on CatalogProvider but ran into TableProvider requiring Debug, waiting on #12557 which covers this to merge.

@AnthonyZhOon
Copy link
Contributor

Finished being able to derive Debug on SessionStateBuilder #12632 but I believe it still needs to implement formatting to be more useful

@alamb
Copy link
Contributor Author

alamb commented Sep 26, 2024

Finished being able to derive Debug on SessionStateBuilder #12632 but I believe it still needs to implement formatting to be more useful

Do you mean "derive a more usable Display" or something?

@AnthonyZhOon
Copy link
Contributor

AnthonyZhOon commented Sep 28, 2024

I think a pretty-print debug works okay using a println!("{:#?}", state), most of the noise in the default was from printing long Vecs of scalar functions which have been moved to the end of debug now.
Noticed that the missing fields in SessionState's Debug was from before we enabled it for the fields, so here's a pull request adding the remaining debug fields #12663 .
It's also possible to just derive Debug for both SessionState and SessionStateBuilder now.

@findepi
Copy link
Member

findepi commented Oct 2, 2024

Done in #12632. The issue can be closed.
thank you @AnthonyZhOon !

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

No branches or pull requests

4 participants