martin-g commented on code in PR #19492:
URL: https://github.com/apache/datafusion/pull/19492#discussion_r2649586826
##########
benchmarks/src/h2o.rs:
##########
@@ -64,15 +62,15 @@ pub struct RunOpt {
/// Path to data files (parquet or csv), using , to separate the paths
/// Default value is the small files for join x table, small table, medium
table, big table files in the h2o benchmark
/// This is the small csv file case
- #[structopt(
- short = "join-paths",
Review Comment:
👍🏻
This seems to have been a bug in the old impl. I guess structopt ignored it
and
[used](https://docs.rs/structopt/latest/structopt/#specifying-argument-types)
the first letter of the field name (`j`)
It is good that Clap uses a char for short!
##########
benchmarks/src/bin/imdb.rs:
##########
@@ -34,24 +34,24 @@ static ALLOC: snmalloc_rs::SnMalloc = snmalloc_rs::SnMalloc;
#[global_allocator]
static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc;
-#[derive(Debug, StructOpt)]
-#[structopt(about = "benchmark command")]
+#[derive(Debug, Subcommand)]
enum BenchmarkSubCommandOpt {
- #[structopt(name = "datafusion")]
+ #[command(name = "datafusion")]
DataFusionBenchmark(imdb::RunOpt),
}
-#[derive(Debug, StructOpt)]
-#[structopt(name = "IMDB", about = "IMDB Dataset Processing.")]
+#[derive(Debug, Parser)]
+#[command(name = "IMDB", about = "IMDB Dataset Processing.")]
enum ImdbOpt {
Review Comment:
This is unconventional - `#[derive(Parser)]` + `enum`.
Let's keep it consistent with the other binaries, e.g.:
```rust
#[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:
##########
@@ -16,34 +16,34 @@
// under the License.
//! mem_profile binary entrypoint
+use clap::{Parser, Subcommand};
use datafusion::error::Result;
use std::{
env,
io::{BufRead, BufReader},
path::Path,
process::{Command, Stdio},
};
-use structopt::StructOpt;
use datafusion_benchmarks::{
clickbench,
h2o::{self, AllQueries},
imdb, sort_tpch, tpch,
};
-#[derive(Debug, StructOpt)]
-#[structopt(name = "Memory Profiling Utility")]
+#[derive(Debug, Parser)]
+#[command(name = "Memory Profiling Utility")]
struct MemProfileOpt {
Review Comment:
nit: Shall it rename it to `Cli` to make it consistent with the other
binaries ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
