Fix the missing first bar in the summary chart - #1789
Merged
Conversation
The baseline for the load time and data size components of the combined
metric was computed over entries passing a strictly-greater threshold,
while the components themselves are applied to entries passing a
greater-or-equal one:
const min_load_time = Math.min(... .filter(x => x && x > 5));
...
Math.log(elem.load_time >= 5 ? (elem.load_time / min_load_time) : 1)
When no selected entry has a load time above 5s, `Math.min` of an empty
list is `Infinity`, so an entry with a load time of exactly 5s got
`5 / Infinity == 0`, and the whole combined score collapsed to
`exp(-Infinity) == 0`. It then sorted to the top and rendered a bar of
zero width.
Reproducer: CedarDB (Parquet) and ClickHouse (Parquet) selected together
on c6a.4xlarge / c7a.metal-48xl / c8g.metal-48xl with the combined
metric - CedarDB (Parquet) on c6a.4xlarge loads in exactly 5s. With this
change it scores x2.44 and sorts last, matching its cold and hot ratios
of 2.24 and 3.37.
Make the thresholds match the conditions that consume them. The load
time row of the detailed table already used `>= 5` for the same purpose.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The first bar does not render in this view (CedarDB (Parquet) + ClickHouse (Parquet), combined metric).
Cause
The baseline for the load time and data size components of the combined metric was computed over entries passing a strictly-greater threshold, while the components themselves are applied to entries passing a greater-or-equal one:
In that selection every load time is 0, 1, 3 or 5 — nothing is
> 5, soMath.minof an empty list returnsInfinity. CedarDB (Parquet) onc6a.4xlargeloads in exactly 5s, which does satisfy>= 5, so its load time factor became5 / Infinity == 0,Math.log(0) == -Infinity, and the whole combined score collapsed toexp(-Infinity) == 0. A ratio of 0 sorts first and renders a bar of zero width.Fix
Make the thresholds match the conditions that consume them. The load time row of the detailed table already used
>= 5for the same purpose, which confirms the intended boundary.With the fix, CedarDB (Parquet) on
c6a.4xlargescores ×2.44 and sorts last, consistent with its cold and hot ratios of 2.24 and 3.37.Effect on the default view
min_load_timegoes from 6 to 5, so entries with a counted load time shift by a uniform (6/5)^0.1 ≈ 1.8%. That swaps a handful of adjacent near-ties but changes nothing structural, and it is the correct baseline — a 5s load was always eligible to be counted.🤖 Generated with Claude Code