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

Derive Debug for SessionStateBuilder, adding Debug requirements to fields #12632

Merged
merged 16 commits into from
Sep 27, 2024

Conversation

AnthonyZhOon
Copy link
Contributor

Which issue does this PR close?

Progress on #12555

Rationale for this change

To make configuration easier to use by providing debug output.

What changes are included in this PR?

  1. Require Debug for CatalogProvider, CatalogProviderList, UrlTableFactory.
  2. Require Debug for ExprPlanner, QueryPlanner.
  3. Require Debug for TableFunctionImpl
  4. Require Debug for SerializerRegistry
  5. Require Debug for FunctionFactory
  6. #derive debug on necessary classes
  7. Implement Debug on SessionStateBuilder by deriving.

Are these changes tested?

By the compiler and CI.

Are there any user-facing changes?

The additional Debug requirement is a user-facing API change on the traits affected.

@github-actions github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate substrait catalog Related to the catalog crate functions labels Sep 26, 2024
@AnthonyZhOon
Copy link
Contributor Author

Needs the api-change tag. I'm also fine with splitting this PR into smaller ones

@AnthonyZhOon AnthonyZhOon changed the title Implement Debug for SessionStateBuilder, adding Debug requirements to fields Derive Debug for SessionStateBuilder, adding Debug requirements to fields Sep 26, 2024
@AnthonyZhOon
Copy link
Contributor Author

Feel like not adding the derive Debug for SessionStateBuilder for now, this is the current output of a SessionStateBuilder::new().with_default_features()
debug_session_state_builder.txt

… keep consistent Debug field order with `SessionState`
@AnthonyZhOon
Copy link
Contributor Author

Now the impl Debug for SessionStateBuilder resembles that of SessionState but includes all fields. Also grouped fields by relevance

@alamb alamb added the api change Changes the API exposed to users of the crate label Sep 26, 2024
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 @AnthonyZhOon -- this looks like a great improvement to me. I agree that the actual utility of the Debug impl is debatable, but at least now it is possible

impl Debug for InformationSchemaConfig {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.debug_struct("InformationSchemaConfig")
// TODO it would be great to print the catalog list here
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@@ -177,23 +177,23 @@ impl Debug for SessionState {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("SessionState")
.field("session_id", &self.session_id)
.field("analyzer", &"...")
.field("config", &self.config)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could simply #derive(Debug) for SessionState now?

And perhaps we could provide a impl Display for SessionState that had a more readable format (like that shows session_id and deviation from default configuration, for example 🤔 )

@alamb alamb merged commit 4df83f5 into apache:main Sep 27, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 27, 2024

Thanks again @AnthonyZhOon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate catalog Related to the catalog crate core Core DataFusion crate functions logical-expr Logical plan and expressions substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants