Uh oh!
There was an error while loading. Please reload this page.
fix: fix panic when lo is greater than hi - #19099
Merged
Merged
Conversation
tshauckforce-pushed
the
fix-panic-in-tdigest
branch
from
December 5, 2025 01:09
2d85ace to
723e035Comparetshauck
commented
Dec 5, 2025
| use datafusion_common::instant::Instant; | ||
| use object_store::{path::Path, ObjectMeta}; | ||
| use object_store::{ObjectMeta, path::Path}; |
ContributorAuthor
There was a problem hiding this comment.
This is unrelated, but was causing the cargo fmt CI job to fail.
Contributor
There was a problem hiding this comment.
👍 @adriangb seems to have fixed this in the recent commit to main
alamb
approved these changes
Dec 5, 2025
| use datafusion_common::instant::Instant; | ||
| use object_store::{path::Path, ObjectMeta}; | ||
| use object_store::{ObjectMeta, path::Path}; |
Contributor
There was a problem hiding this comment.
👍 @adriangb seems to have fixed this in the recent commit to main
tshauckforce-pushed
the
fix-panic-in-tdigest
branch
from
December 5, 2025 15:31
723e035 to
f4b0e2cComparetshauck
marked this pull request as ready for review
December 5, 2025 15:31
alamb
reviewed
Dec 5, 2025
| } | ||
| #[test] | ||
| fn test_identical_values_floating_point_precision() { |
Contributor
There was a problem hiding this comment.
I ran this test on my (mac) without the code change I verified it fails
thread 'tdigest::tests::test_identical_values_floating_point_precision' (9627513) panicked at /Users/andrewlamb/.rustup/toolchains/1.91.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/num/f64.rs:1413:9:
min > max, or either was NaN. min = 15.699999988079075, max = 15.699999988079073
stack backtrace:
I also double checked the original query it is good now:
DataFusion CLI v51.0.0
>selectapprox_percentile_cont(0.99) within group (order by value) from (select 15.699999988079073 as value from generate_series(1,215));
+---------------------------------------------------------------------------+
| approx_percentile_cont(Float64(0.99)) WITHIN GROUP [value ASC NULLS LAST] |
+---------------------------------------------------------------------------+
| 15.699999988079075 |
+---------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.035 seconds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
It's possible w/ floating point issues that lo could be greater than hi, causing a panic in clamp.
What changes are included in this PR?
Add a check in clamp that reorders the clamp parameters if they're not in order.
Are these changes tested?
Adds a regression test.
Are there any user-facing changes?
No