You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datasketches.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2026/05/09 06:40:53 UTC
[PR] refactor: gate sketches with feature flags (datasketches-rust)
tisonkun opened a new pull request, #120:
URL: https://github.com/apache/datasketches-rust/pull/120
This closes #32
Some unused warnings and style issues can be fixed later.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3213313777
##########
CONTRIBUTING.md:
##########
@@ -27,7 +27,7 @@ Rustup will read the `rust-toolchain.toml` file and set up everything else autom
```shell
cargo version
-# cargo 1.85.0 (<hash> 2024-12-31)
+# cargo 1.86.0 (<hash> <date>)
Review Comment:
This example output now contains placeholders (`<hash> <date>`), which makes it hard for contributors to compare against their actual `cargo version` output. Consider either keeping a real sample (like before) or removing the parenthesized portion entirely.
##########
xtask/src/main.rs:
##########
@@ -100,18 +146,35 @@ fn run_command(mut cmd: StdCommand) {
assert!(status.success(), "command failed: {status}");
}
-fn make_test_cmd(no_capture: bool, features: &[&str]) -> StdCommand {
+fn make_test_cmd<T: AsRef<str>>(no_capture: bool, features: &[T]) -> StdCommand {
let mut cmd = find_command("cargo");
cmd.args(["test", "--workspace", "--no-default-features"]);
- if !features.is_empty() {
- cmd.args(["--features", features.join(",").as_str()]);
+ for feature in features {
+ cmd.args(["--features", feature.as_ref()]);
}
if no_capture {
cmd.args(["--", "--nocapture"]);
}
cmd
}
+fn make_check_cmd<T: AsRef<str>>(features: &[T]) -> StdCommand {
+ let mut cmd = find_command("cargo");
+ cmd.env("RUSTFLAGS", "-Dwarnings");
+ cmd.args([
+ "+nightly",
+ "check",
+ "--package",
+ "datasketches",
+ "--all-targets",
+ "--no-default-features",
+ ]);
Review Comment:
The new `x check` subcommand forces `cargo +nightly check`, but the CI test job installs only `${{ matrix.rust-version }}` (MSRV/stable) and then runs `cargo x check`. This will fail because the nightly toolchain isn’t installed, and it also defeats the purpose of checking against MSRV. Prefer running `cargo check` on the currently-selected toolchain (drop the `+nightly` override), or ensure CI installs nightly in the test matrix if nightly is truly required.
##########
datasketches/Cargo.toml:
##########
@@ -34,6 +34,18 @@ keywords = ["sketch", "hyperloglog", "probabilistic"]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]
+[features]
+default = []
+
+# Each sketch has its own feature, so that users can opt in to only the sketches they need.
+bloom = []
+countmin = []
+cpc = []
+frequencies = []
+hll = []
+tdigest = []
+theta = []
+
Review Comment:
`datasketches` now defines per-sketch Cargo features, but `default = []` contradicts the Issue #32 requirement (“By default, all sketches should be turned on”). With the current defaults, users upgrading will lose all sketch modules unless they opt-in via features (breaking change). Consider making `default` include all sketch features (or add an `all` feature and set `default = ["all"]`).
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#issuecomment-4412875689
Thanks for your review @PsiACE!
I'm going to merge this and cut a 0.3.0 release candidate in the next Monday. So I'd appreciate it if @notfilippo @ZENOTME can drop a review so that we don't need to go back for hotfix after the RC is out.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#issuecomment-4412877412
Well. I think I can merge this now and prepare the changelog and other setups. And feedback can steer in to break the release process :D
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3213341135
##########
datasketches/Cargo.toml:
##########
@@ -34,6 +34,18 @@ keywords = ["sketch", "hyperloglog", "probabilistic"]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]
+[features]
+default = []
+
+# Each sketch has its own feature, so that users can opt in to only the sketches they need.
+bloom = []
+countmin = []
+cpc = []
+frequencies = []
+hll = []
+tdigest = []
+theta = []
Review Comment:
`default = []` makes the crate build with *no* sketches enabled unless the caller opts into features, which is a breaking change for existing users and conflicts with issue #32’s requirement that “by default, all sketches should be turned on”. Consider setting `default` to include all sketch features (or introducing an `all` feature that’s included in `default`).
##########
CONTRIBUTING.md:
##########
@@ -27,7 +27,7 @@ Rustup will read the `rust-toolchain.toml` file and set up everything else autom
```shell
cargo version
-# cargo 1.85.0 (<hash> 2024-12-31)
+# cargo 1.86.0 (<hash> <date>)
Review Comment:
The `cargo version` example output uses placeholders (`<hash> <date>`), which doesn’t match the actual format users will see and can be confusing when validating their toolchain setup. Consider using a real example string or removing the parenthesized portion entirely.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3212656238
##########
datasketches/Cargo.toml:
##########
@@ -34,6 +34,18 @@ keywords = ["sketch", "hyperloglog", "probabilistic"]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]
+[features]
+default = []
+
+# Each sketch has its own feature, so that users can opt in to only the sketches they need.
+bloom = []
+countmin = []
+cpc = []
Review Comment:
`default = []` means none of the sketch modules are enabled by default, so `datasketches::theta`/`hll`/etc disappear from the public API unless users opt-in via features. This is a breaking change and also doesn’t match the linked issue’s stated goal that all sketches be on by default. Consider setting `default` to include all sketch features (or define an `all` feature and make it the default).
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3213307748
##########
Cargo.toml:
##########
@@ -35,7 +35,7 @@ datasketches = { path = "datasketches" }
clap = { version = "4.5.20", features = ["derive"] }
insta = { version = "1.46.1" }
googletest = { version = "0.14.2" }
-rand = { version = "0.9.2" }
+cargo_metadata = { version = "0.19.1" }
Review Comment:
Update MSRV to 1.86.0 to simplify workflow.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3213285458
##########
Cargo.toml:
##########
@@ -35,7 +35,7 @@ datasketches = { path = "datasketches" }
clap = { version = "4.5.20", features = ["derive"] }
insta = { version = "1.46.1" }
googletest = { version = "0.14.2" }
-rand = { version = "0.9.2" }
+cargo_metadata = { version = "0.19.1" }
Review Comment:
Latest version 0.23.1 requires 1.86 and the main branch bumps MSRV to 1.88.
I think we can anyway consider bump MSRV later.
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] refactor: gate sketches with feature flags (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120
--
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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org