Skip to content

Trying cargo machete to prune unused deps. #17545

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 15, 2025
Merged

Trying cargo machete to prune unused deps. #17545

merged 2 commits into from
Sep 15, 2025

Conversation

@samueleresca
Copy link
Member

@samueleresca samueleresca commented Sep 12, 2025

Very much a test with cargo machete to understand all the false positives detected by the tooling.

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate catalog Related to the catalog crate proto Related to proto crate datasource Changes to the datasource crate physical-plan Changes to the physical-plan crate spark labels Sep 12, 2025
@samueleresca samueleresca force-pushed the test-machete branch 2 times, most recently from 3ea9fd5 to 0977b19 Compare September 12, 2025 20:42
@samueleresca samueleresca mentioned this pull request Sep 13, 2025
Closed
@alamb
Copy link
Contributor

alamb commented Sep 14, 2025

Looks like the experiment was a success ❤️

alamb
alamb approved these changes Sep 14, 2025
View reviewed changes
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.

Looks great to me -- thank you @samueleresca

@alamb alamb marked this pull request as ready for review September 14, 2025 11:30
@alamb
Copy link
Contributor

alamb commented Sep 14, 2025

Anything else you think we should do with this PR @samueleresca ? I marked it ready for review as it looked great

Jefffrey
Jefffrey reviewed Sep 14, 2025
View reviewed changes
datafusion/datasource/Cargo.toml Outdated

[features]
parquet = ["dep:parquet", "tempfile"]
parquet = ["tempfile"]
Copy link
Contributor

@Jefffrey Jefffrey Sep 14, 2025

Choose a reason for hiding this comment

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

I guess remove parquet feature from here as well?

Copy link
Member Author

@samueleresca samueleresca Sep 15, 2025

Choose a reason for hiding this comment

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

Proceeding by removing the parquet feature. Reasoning:

  • tempfile is already included as part of the dependencies.
  • There isn't any conditional code linked to the parquet features.

Thanks

@github-actions github-actions bot added documentation Improvements or additions to documentation sql SQL Planner sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Sep 15, 2025
@github-actions github-actions bot removed documentation Improvements or additions to documentation sql SQL Planner sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Sep 15, 2025
Jefffrey
Jefffrey approved these changes Sep 15, 2025
View reviewed changes
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Nice cleanup

@samueleresca
Copy link
Member Author

samueleresca commented Sep 15, 2025

Anything else you think we should do with this PR @samueleresca ? I marked it ready for review as it looked great

Thanks 😄 I think we can proceed with this PR. I might open another PR for including cargo machete checks in CI. Full discussion in: #17550

@alamb alamb merged commit 3f22f8c into apache:main Sep 15, 2025
30 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 15, 2025

Anything else you think we should do with this PR @samueleresca ? I marked it ready for review as it looked great

Thanks 😄 I think we can proceed with this PR. I might open another PR for including cargo machete checks in CI. Full discussion in: #17550

Thanks @samueleresca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@Jefffrey Jefffrey Jefffrey approved these changes

@xudong963 xudong963 xudong963 approved these changes

Assignees

No one assigned

Labels

catalog Related to the catalog crate core Core DataFusion crate datasource Changes to the datasource crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate proto Related to proto crate spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@samueleresca @alamb @Jefffrey @xudong963