fix(dendrogram): pass the color threshold to scipy as a number - #7234
Merged
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7234 +/- ##
============================================
+ Coverage 80.58% 82.08% +1.50%
- Complexity 3890 4078 +188
============================================
Files 1162 1162
Lines 46264 46302 +38
Branches 5153 5159 +6
============================================
+ Hits 37280 38009 +729
+ Misses 7319 6584 -735
- Partials 1665 1709 +44
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 403 | 0.246 | 23,584/35,490/35,490 us | 🔴 +20.0% / 🔴 +126.1% |
| 🔴 | bs=100 sw=10 sl=64 | 941 | 0.574 | 104,646/133,274/133,274 us | 🔴 +16.2% / 🔴 +24.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,106 | 0.675 | 906,259/953,581/953,581 us | ⚪ within ±5% / 🟢 -9.9% |
Baseline details
Latest main 436b37e from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 403 tuples/sec | 462 tuples/sec | 782.98 tuples/sec | -12.8% | -48.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.246 MB/s | 0.282 MB/s | 0.478 MB/s | -12.8% | -48.5% |
| bs=10 sw=10 sl=64 | p50 | 23,584 us | 20,943 us | 12,467 us | +12.6% | +89.2% |
| bs=10 sw=10 sl=64 | p95 | 35,490 us | 29,583 us | 15,695 us | +20.0% | +126.1% |
| bs=10 sw=10 sl=64 | p99 | 35,490 us | 29,583 us | 18,950 us | +20.0% | +87.3% |
| bs=100 sw=10 sl=64 | throughput | 941 tuples/sec | 972 tuples/sec | 996.92 tuples/sec | -3.2% | -5.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.574 MB/s | 0.593 MB/s | 0.608 MB/s | -3.2% | -5.7% |
| bs=100 sw=10 sl=64 | p50 | 104,646 us | 101,914 us | 100,899 us | +2.7% | +3.7% |
| bs=100 sw=10 sl=64 | p95 | 133,274 us | 114,697 us | 107,385 us | +16.2% | +24.1% |
| bs=100 sw=10 sl=64 | p99 | 133,274 us | 114,697 us | 115,226 us | +16.2% | +15.7% |
| bs=1000 sw=10 sl=64 | throughput | 1,106 tuples/sec | 1,108 tuples/sec | 1,024 tuples/sec | -0.2% | +8.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.675 MB/s | 0.676 MB/s | 0.625 MB/s | -0.1% | +8.0% |
| bs=1000 sw=10 sl=64 | p50 | 906,259 us | 909,902 us | 989,300 us | -0.4% | -8.4% |
| bs=1000 sw=10 sl=64 | p95 | 953,581 us | 965,359 us | 1,031,689 us | -1.2% | -7.6% |
| bs=1000 sw=10 sl=64 | p99 | 953,581 us | 965,359 us | 1,058,394 us | -1.2% | -9.9% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,495.89,200,128000,403,0.246,23583.95,35490.07,35490.07
1,100,10,64,20,2125.70,2000,1280000,941,0.574,104646.33,133273.50,133273.50
2,1000,10,64,20,18085.26,20000,12800000,1106,0.675,906259.07,953580.92,953580.92Color Threshold could not be set at all. The field was declared as a string and spliced into the generated Python as a decode expression, so scipy received the Python string '3' rather than 3 and raised UFuncTypeError comparing it against the linkage distances. The only values that plotted were a blank field and the literal "default", and scipy documents those as equivalent -- both mean 0.7 * max distance -- so nothing a user could type ever changed the coloring. Declaring the field numeric fixes it: the number is spliced as a literal, so scipy gets a number, and an unset threshold stays None, which is the same 0.7 * max distance a blank field already meant. `@JsonDeserialize(contentAs = ...)` is required and names the boxed class on purpose. Scala erases `Option[Double]`'s element type, so without the annotation Jackson leaves the raw JSON value inside the Option and the first use throws ClassCastException; with the primitive class instead, a blank coerces to 0, which colors every link the same rather than meaning "unset". Workflows that stored a number keep working -- a numeric string still reads as a number, and those workflows were failing before this change anyway. A workflow that stored the literal "default" no longer loads; clearing the field plots the identical chart. Fixes apache#7232 Generated-by: Claude Code (claude-opus-5[1m])
kz930
force-pushed
the
fix/7232-dendrogram-threshold
branch
from
August 2, 2026 04:00
e3feb37 to
90a8871
Compare
aglinxinyuan
approved these changes
Aug 3, 2026
Contributor
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
What changes were proposed in this PR?
Dendrogram's Color Threshold could not be set at all:
'3'rather than3and raisedUFuncTypeErrorcomparing it against the linkage distancesdefault, and scipy documents those as equivalent — both mean 0.7 × max distance — so nothing a user could type ever changed the coloringThis PR declares the field as
Option[Double]:None, the same 0.7 × max distance a blank field already meant@JsonDeserialize(contentAs = ...)names the boxed class: Scala erasesOption's element type, so without it Jackson leaves the raw JSON value inside the Option and the first use throwsClassCastException, and the primitive class would read a blank as 0 — every link colored the same rather than "unset"Compatibility: a numeric string saved earlier still reads as a number, and those workflows were failing before this change anyway. A workflow that stored the literal
defaultno longer loads; clearing the field plots the identical chart.Any related issues, documentation, discussions?
Fixes #7232.
How was this PR tested?
color_threshold=42.5, not as a decoded stringcolor_threshold=3.0plots a figure and so does the unset case, while passing the same value as a string, which is what the operator does today, raisesUFuncTypeErrorscalafmtCheckcleanWas this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5[1m])