You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datasketches.apache.org by "freakyzoidberg (via GitHub)" <gi...@apache.org> on 2025/12/17 14:54:06 UTC
[I] Replace asserts with Result (datasketches-rust)
freakyzoidberg opened a new issue, #27:
URL: https://github.com/apache/datasketches-rust/issues/27
The library should be mindful that the input may not be valid for various reasons.
We generally avoid relying in assert/panics/abort in the library.
The Rust implementation should most likely favor Result<T, E> return instead of assert!
--
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.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: [I] Replace asserts with Result (datasketches-rust)
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665926052
Would there be any reason for a user to dynamically set `lg_config_k` instead of setting it via a constant?
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665738490
For now we've used `unreachable!` for invariants, which I reckon is a correct way of handling dev errors. The concrete use case we have for this is an `InvalidParameter` when creating an `HllSketch` and `HllUnion`.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665943126
> Would there be any reason for a user to dynamically set `lg_config_k` instead of setting it via a constant?
It's technically viable, so as datasketches-rust becomes more popular, someone will use it in this way. But then it's the user's responsibility to check the data they accept.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665786077
`assert!` is used for checking invariants. When an assertion fails, it means an internal bug, and the program should crash instead of continuing in undefined behavior.
It's impossible to change it to `Result<T, E>`. Because it should never fail, if you know the bug, you will fix it rather than return `Err(Bug)`.
I do understand some situations where avoiding untrusted input can crash your program, like in deserialization. One should always use `Err(SerdeError)` rather than panic, because they are known failure cases.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665881332
Take another example, we now have:
```rust
impl HllSketch {
/// # Panics
///
/// If lg_config_k is not in range [4, 21]
pub fn new(lg_config_k: u8, hll_type: HllType) -> Self {
assert!(
lg_config_k > 4 && lg_config_k < 21,
"lg_config_k must be in [4, 21]"
);
}
}
```
(N.B. @notfilippo This seems a bug that it should be `lg_config_k >= 4 && lg_config_k <= 21`)
Users will write:
```rust
// check lg_config_k before hand by any means; typically, it is a user specified constant.
// then ..
let hll = HllSketch::new(DEFAULT_HLL_K);
```
What if we change the assert to `Result<Self, E>`?
```rust
impl HllSketch {
pub fn new(lg_config_k: u8, hll_type: HllType) -> Result<Self, SomeError> {
if lg_config_k < 4 || lg_config_k > 21 { return SomeError; }
}
}
```
Then users will write:
```rust
// check lg_config_k before hand by any means
let hll = HllSketch::new(DEFAULT_HLL_K).unwrap();
```
Some Rust method `foo` would have a `try_foo` variant to return SomeError here instead of letting the caller check the contract. But in many cases, the contract can be checked.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665812881
That is, code like `NonZeroUsize::new(1).unwrap()` should be acceptable. Nothing is expected to happen on the `None` variant because it won't happen.
Actually, when you're using Rust's stdlib, there are already many `assert!` inside (`a[idx]` would panic if index out of bound). Crates like [nonempty-collections](https://github.com/fosskers/nonempty-collections) hide the assert/unwrap for you because it keeps the contracts internally. If they can write sound code with assertions to check invariants, we should be able to do that as well.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "freakyzoidberg (via GitHub)" <gi...@apache.org>.
freakyzoidberg commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3678669245
sounds good. thanks
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "freakyzoidberg (via GitHub)" <gi...@apache.org>.
freakyzoidberg commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3666670201
Sorry, yeah I may have been too directive here.
What I think we want is guarantee that we cannot assert/panic due to corrupted/bad input
for logical errors that can happen independently of the input data - then yes - and we do have a few of those FWIW
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3678386165
@freakyzoidberg we may close this issue as consensus achieved.
Or any other action you expect to do?
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3665900220
> (N.B. @notfilippo This seems a bug that it should be lg_config_k >= 4 && lg_config_k <= 21)
Just fixed in main, caught it with a test! https://github.com/apache/datasketches-rust/commit/4f5f2665b590be992e75eb5d6d5f8ee6b2cc75b4
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3667520299
> Sorry, yeah I may have been too directive here.
>
> What I think we want is guarantee that we cannot assert/panic due to corrupted/bad input
>
> For logical errors that can happen independently of the input data - then yes - and we do have a few of those FWIW
Agree.
I think what we can do here, besides a consensus now, is to document it as a `DESIGN.md`/`STYLE.md` or even in `AGENTS.md` for discoverability :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: [I] Replace asserts with Result (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #27:
URL: https://github.com/apache/datasketches-rust/issues/27#issuecomment-3666011425
That said, I totally agree to use `Result<T, E>` for known fallible cases.
But I've experienced a `Result` everywhere policy applied once, even in cases where it shouldn't. In that story, the codebase ends up with `unwrap` everywhere and panic remains, even more subtle to debug.
One reason here is that Rust doesn't have a global ancient `Exception`/`RuntimeException` class. It doesn't have `error` interface as in Go. So converting `ErrorTypeA` to `ErrorTypeB` among code paths is very awkward. You can use [`anyhow`](https://docs.rs/anyhow/latest/anyhow/trait.Context.html) but then you just know "something happen" rather than handling the Error when you can.
One might consider creating a large error enum to cover all possible fallible cases in the codebase, but that would result in a scary big mess. Actually, I'm feeling the current `SerdeError` a bit over-designed already. Because what users can do for every variant is know that the data is illegal. It can have only one variant with a (debug) cause message to be printed.
However, it's possible to design an `Error` type for a Rust library and use it library-wise. Apache OpenDAL's [`Error`](https://docs.rs/opendal/latest/opendal/struct.Error.html) does a good job of covering all common causes when accessing a storage. datasketches-rust may find such an Error, so most of the fallible methods can use that `Error`.
Besides, Rust has more tools to handle fallible cases. For example, in the TDigest PR, I return `None` if the TDigest is empty on `rank`/`quantile`/`cdf`/`pmf`/`max_value`/`min_value`, while in C++ and Java impl, they throw an `SketchesStateException`.
--
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: [I] Replace asserts with Result (datasketches-rust)
Posted by "freakyzoidberg (via GitHub)" <gi...@apache.org>.
freakyzoidberg closed issue #27: Replace asserts with Result<T, E>
URL: https://github.com/apache/datasketches-rust/issues/27
--
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