You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datasketches.apache.org by "VedantMadane (via GitHub)" <gi...@apache.org> on 2026/08/24 15:56:09 UTC
[PR] feat: return Option from intersection to_sketch before first update (datasketches-rust)
VedantMadane opened a new pull request, #211:
URL: https://github.com/apache/datasketches-rust/pull/211
## Summary
feat: return Option from intersection to_sketch before first update
## Changes
- ThetaIntersection::to_sketch and TupleIntersection::to_sketch return Option
- tests: expect None instead of panic before first update
Fixes #192
--
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] feat: return Option from intersection to_sketch before first update (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #211:
URL: https://github.com/apache/datasketches-rust/pull/211#discussion_r3848205912
##########
datasketches/src/thetafamily/tuple/intersection.rs:
##########
@@ -79,7 +79,7 @@ use crate::tuple::sketch::TupleSketchView;
/// intersection.update(&a).unwrap();
/// intersection.update(&b).unwrap();
///
-/// let result = intersection.to_sketch(true);
+/// let result = intersection.to_sketch(true).expect("after update");
Review Comment:
```suggestion
/// let result = intersection.to_sketch(true).unwrap();
```
--
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] feat: return Option from intersection to_sketch before first update (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #211:
URL: https://github.com/apache/datasketches-rust/pull/211#discussion_r3848232901
##########
datasketches/tests/tuple_test/intersection.rs:
##########
@@ -49,14 +49,13 @@ fn has_result_tracks_the_first_update() {
assert!(!intersection.has_result());
intersection.update(&sketch).unwrap();
assert!(intersection.has_result());
- assert_eq!(intersection.to_sketch(true).num_retained(), 10);
+ assert_eq!(intersection.to_sketch(true).unwrap().num_retained(), 10);
}
#[test]
-#[should_panic(expected = "before first update")]
-fn result_before_first_update_panics() {
- let intersection = TupleIntersection::new(SumPolicy);
- let _ = intersection.to_sketch(true);
+fn result_before_first_update_returns_none() {
+ let intersection = TupleIntersection::<IdentityPolicy>::new();
Review Comment:
```suggestion
let intersection = TupleIntersection::new(SumPolicy);
```
LLM Hallucination?
--
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] feat: return Option from intersection to_sketch before first update (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #211:
URL: https://github.com/apache/datasketches-rust/pull/211#discussion_r3848833265
##########
datasketches/src/thetafamily/theta/intersection.rs:
##########
@@ -75,16 +75,15 @@ impl ThetaIntersection {
/// Returns the intersection result as a compact theta sketch.
///
- /// # Panics
- ///
- /// Panics if called before the first [`update`](Self::update).
- pub fn to_sketch(&self, ordered: bool) -> CompactThetaSketch {
- assert!(
- self.state.has_result(),
- "ThetaIntersection::to_sketch() called before first update()"
- );
+ /// Returns `None` if called before the first [`update`](Self::update).
+ /// Absence of a result is part of the public state machine; use
+ /// [`has_result`](Self::has_result) or this `Option` return instead of panicking (#192).
Review Comment:
Addressed in d1aaa26. I rewrote the Theta and Tuple docs around the observable contract: a new intersection returns `None`; after a successful update it returns `Some`, including when the intersection is empty. I also removed the issue reference and aligned the stale Theta test name.
--
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] feat: return Option from intersection to_sketch before first update (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #211:
URL: https://github.com/apache/datasketches-rust/pull/211#discussion_r3848755772
##########
datasketches/src/thetafamily/theta/intersection.rs:
##########
@@ -75,16 +75,15 @@ impl ThetaIntersection {
/// Returns the intersection result as a compact theta sketch.
///
- /// # Panics
- ///
- /// Panics if called before the first [`update`](Self::update).
- pub fn to_sketch(&self, ordered: bool) -> CompactThetaSketch {
- assert!(
- self.state.has_result(),
- "ThetaIntersection::to_sketch() called before first update()"
- );
+ /// Returns `None` if called before the first [`update`](Self::update).
+ /// Absence of a result is part of the public state machine; use
+ /// [`has_result`](Self::has_result) or this `Option` return instead of panicking (#192).
Review Comment:
We should improve this doc; at least we should not suddenly tell `#192` or similar thing that is too detailed.
--
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] feat: return Option from intersection to_sketch before first update (datasketches-rust)
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #211:
URL: https://github.com/apache/datasketches-rust/pull/211
--
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