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

Remove unused dependencies and features #12808

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Conversation

jonahgao
Copy link
Member

@jonahgao jonahgao commented Oct 8, 2024

Which issue does this PR close?

N/A

Rationale for this change

Found by cargo-udeps. There are some unused dependencies in Cargo.toml

The regex_expressions and encoding_expressions feature originate from the datafusion-functions package.
The datafusion-physical-expr package doesn’t depend on datafusion-functions, so I think it should not incorporate these two features.

What changes are included in this PR?

  • Remove unused dependencies
  • Remove regex_expressions and encoding_expressions feature from datafusion-physical-expr

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels Oct 8, 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 @jonahgao -- this is a nice cleanup

@@ -69,6 +68,7 @@ rand = { workspace = true }
tokio = { workspace = true }

[dev-dependencies]
datafusion-functions-aggregate = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice

As a follow on it would be great to remove this dependency too, but having it as a dev dependency is clearly better than a real dependency in my mind

Copy link
Member Author

Choose a reason for hiding this comment

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

Some tests require an aggregate function and will check the aggregation results, so not sure if it's possible to remove this dependency 🤔.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move that test into the sql_integration binary or something

Copy link
Member Author

@jonahgao jonahgao Oct 9, 2024

Choose a reason for hiding this comment

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

I found that some tests call private methods, which means we can't move them to a different package. Perhaps we can try to address this in a subsequent PR.

@alamb alamb merged commit 3d347c9 into apache:main Oct 9, 2024
26 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 9, 2024

Thanks @jonahgao and @findepi

@jonahgao jonahgao deleted the remove_unused branch October 9, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate optimizer Optimizer rules physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants