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

Require Debug for PhysicalOptimizerRule #12624

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Require Debug for PhysicalOptimizerRule #12624

merged 4 commits into from
Sep 25, 2024

Conversation

AnthonyZhOon
Copy link
Contributor

@AnthonyZhOon AnthonyZhOon commented Sep 25, 2024

Which issue does this PR close?

Part of #12555

Rationale for this change

Supporting SessionStateBuilder to implement Debug

What changes are included in this PR?

  1. Require Debug for PhysicalOptimizerRule
  2. #derive Debug where necessary
    3. Add a & to take a &JoinType in EnforceDistribution (not sure why this wasn't a cargo check error but rust-analyzer showed it and types seem to require it)

Are these changes tested?

By CI and the compiler

Are there any user-facing changes?

API change for PhysicalOptimizerRule which now requires Debug

@github-actions github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Sep 25, 2024
@AnthonyZhOon
Copy link
Contributor Author

Need to add the api-change label

@alamb alamb added the api change Changes the API exposed to users of the crate label Sep 25, 2024
@alamb
Copy link
Contributor

alamb commented Sep 25, 2024

Thank you @AnthonyZhOon -- this looks super helpful

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.

Love it -- thank you @AnthonyZhOon

@alamb
Copy link
Contributor

alamb commented Sep 25, 2024

Looks like there are some small CI failures to address @AnthonyZhOon

… informs this is unnecessary

This reverts commit f69d73c.
@AnthonyZhOon
Copy link
Contributor Author

Okay, clippy lint should be fixed with the revert @alamb , odd that rust-analyzer misled me on the types at the time maybe it was an incomplete compile

@alamb
Copy link
Contributor

alamb commented Sep 25, 2024

I think this one is minor enough and non controversial I will merge it in, even though it is an API change

@alamb alamb merged commit 91c8a47 into apache:main Sep 25, 2024
25 checks passed
bgjackma pushed a commit to bgjackma/datafusion that referenced this pull request Sep 25, 2024
* Require Debug for PhysicalOptimizerRule

* Add reference to meet &JoinType type required

* Revert "Add reference to meet &JoinType type required" as clippy lint informs this is unnecessary

This reverts commit f69d73c.
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 core Core DataFusion crate optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants