Replace deprecated structopt with clap in datafusion-benchmarks #19492
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
Contributor
Author
|
@Jefffrey could u review this one !? |
Contributor
|
I'll try take a look when I have time |
martin-g
reviewed
Dec 28, 2025
| #[structopt(name = "IMDB", about = "IMDB Dataset Processing.")] | ||
| #[derive(Debug, Parser)] | ||
| #[command(name = "IMDB", about = "IMDB Dataset Processing.")] | ||
| enum ImdbOpt { |
Member
There was a problem hiding this comment.
This is unconventional - #[derive(Parser)] + enum.
Let's keep it consistent with the other binaries, e.g.:
#[derive(Debug, Parser)]
struct Cli {
#[command(subcommand)]
command: ImdbOpt,
}
#[derive(Debug, Subcommand)]
enum ImdbOpt {
Benchmark(BenchmarkSubCommandOpt),
Convert(imdb::ConvertOpt),
}
...
benchmarks/src/bin/mem_profile.rs
Outdated
| #[structopt(name = "Memory Profiling Utility")] | ||
| #[derive(Debug, Parser)] | ||
| #[command(name = "Memory Profiling Utility")] | ||
| struct MemProfileOpt { |
Member
There was a problem hiding this comment.
nit: Shall it rename it to Cli to make it consistent with the other binaries ?
Yuvraj-cyborg
force-pushed
the
replace-structopt-with-clap
branch
from
721fd14 to
e6d0f01
Compare
Yuvraj-cyborg
force-pushed
the
replace-structopt-with-clap
branch
from
e6d0f01 to
70fc238
Compare
Member
|
Please don't force-push! It makes it harder to make incremental reviews. |
Contributor
Author
Ahh sorry bad habit of rebasing and force pushing, would take care of it from next time, apologies ! |
Contributor
|
run benchmarks |
|
🤖 |
Contributor
|
Thanks @Yuvraj-cyborg and @martin-g |
alamb
approved these changes
Dec 30, 2025
| "libloading 0.8.9", | ||
| ] | ||
|
|
||
| [[package]] |
Contributor
There was a problem hiding this comment.
this is really nice to see ❤️
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the datafusion-benchmarks crate from the deprecated structopt library to clap v4 with derive features, removing an unmaintained dependency and modernizing the CLI parsing approach.
Key Changes
- Replaced structopt dependency with clap 4.5.53 featuring derive macros
- Migrated all CLI argument parsing code across 16+ source files to use clap's derive API
- Updated all attribute syntax from structopt conventions to clap v4 patterns
Reviewed changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| benchmarks/Cargo.toml | Adds clap 4.5.53 with derive features, removes structopt dependency |
| Cargo.lock | Removes structopt and its transitive dependencies (clap 2.x, heck 0.3.3, proc-macro-error, etc.), consolidates on clap 4.5.53 |
| benchmarks/src/util/options.rs | Migrates CommonOpt from StructOpt to Args, updates custom parser to use value_parser |
| benchmarks/src/tpch/run.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/tpcds/run.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/sort_tpch.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/smj.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/nlj.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/imdb/run.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/imdb/convert.rs | Converts ConvertOpt to use Args derive with updated attribute syntax |
| benchmarks/src/hj.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/h2o.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/clickbench.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/cancellation.rs | Converts RunOpt to use Args derive with updated attribute syntax |
| benchmarks/src/bin/mem_profile.rs | Restructures CLI with Parser and Subcommand derives, uses Cli::parse() |
| benchmarks/src/bin/imdb.rs | Restructures CLI with Parser and Subcommand derives for nested subcommands, uses Cli::parse() |
| benchmarks/src/bin/external_aggr.rs | Restructures CLI with Parser, Subcommand, and Args derives, uses Cli::parse() |
| benchmarks/src/bin/dfbench.rs | Restructures CLI with Parser and Subcommand derives, uses Cli::parse() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
I ran the tests locally and they worked well for me too |
|
🤖: Benchmark completed Details
|
Contributor
|
I'll plan to merge this tomorrow unless anyone else would like time to review |
Contributor
|
Thanks again @Yuvraj-cyborg and @martin-g |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The structopt crate is deprecated and has been superseded by clap with its derive feature.
This migration:
What changes are included in this PR?
Updated Cargo.toml to use clap = { version = "4.5.53", features = ["derive"] } instead of structopt
Migrated all benchmark source files to use clap's derive macros:
#[derive(StructOpt)]→#[derive(Parser/Args/Subcommand)]#[structopt(...)]→#[arg(...)]for fields#[structopt(...)]→#[command(...)]for struct/enum-level attributesStructOpt::from_args()→Parser::parse()Removed
parse(from_os_str)as PathBuf works natively in clapChanged short flag strings to chars ('p' instead of "p")
Are these changes tested?
Yes
Are there any user-facing changes?
No. The CLI interface remains identical - this is a drop-in replacement of the underlying argument parsing library.