feat: tpchgen runners as a lib #202
Conversation
|
Thanks @clflushopt! No comments on my side, seems quite straight forward to use! |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @clflushopt -- this is a really neat idea
it looks like some of the CI tests are failing, and I have a few API suggestions (but nothing I think is a deal breaker or that we couldn't do as a follow on PR
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn generate(self) -> io::Result<()> { |
There was a problem hiding this comment.
if the only function on TpchGenerator is generate() we can probably simplify the API by consolidating the builder and generator
So instead of
TpchGenerator::builder()
.scale_factor(1.0)
.build()
.generate()
.await?;Something like
TpchGenerator::new()
.scale_factor(1.0)
.generate()
.await?;There was a problem hiding this comment.
While trying to address this I realized that without a builder/generator separation, handling options in the main Cli becomes a bit cumbersome. Any thoughts ?
There was a problem hiding this comment.
I think your examples look good now to me and I don't have any better ideas. So let's 🚢 🇮🇹 and if we come up with something better we can make another release
| /// ```no_run | ||
| /// use tpchgen_cli::{GeneratorConfig, OutputFormat}; | ||
| /// | ||
| /// // Usually you would use TpchGenerator::builder() instead |
There was a problem hiding this comment.
If we provide a Builder API what is the usecase for the config as well?
If we made the GeneratorConfig non public, the API becomes simpler I think, and we could add new fields without causing breaking changes in the future
Or put another way, I feel like we should have either GeneratorConfig or TpchGeneratorBuilder but not both 🤔
There was a problem hiding this comment.
Thinking along the same lines, I think we can deprecate GeneratorConfig in favor of exposing the builder API.
tpchgen-cli/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Set the scale factor (e.g., 1.0 for 1GB, 10.0 for 10GB) | ||
| pub fn scale_factor(mut self, scale_factor: f64) -> Self { |
There was a problem hiding this comment.
I personally recommend:
- Adding accessor for all these properties (e.g. scale_factor() returns
f64) - Changing the setters for the builder api to be named using
with_prefixes -- e.g.with_scale_factor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @clflushopt -- this makes sense to me
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn generate(self) -> io::Result<()> { |
There was a problem hiding this comment.
I think your examples look good now to me and I don't have any better ideas. So let's 🚢 🇮🇹 and if we come up with something better we can make another release
Exposes the CLI core runner capabilities as a library allowing it to be ended rather easily.