fix: name SQL summary state columns independently of readout parameters - #516
Merged
Merged
Conversation
A quantile summary's state column was named after the query's output column (e.g. `approx_percentile_cont(t.x,Float64(0.5))`, or PromQL's cross-series `quantile_0_5`), so p50 and p99 over one input built structurally different states and post-ASAP CSE could not share them. Name the state after the column it summarizes; the readout keeps the query's output name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <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.
Why
After #515, structurally identical summary producers are shared across queries. SQL p50 and p99 over the same table, filter and column were still not shared, and neither were PromQL cross-series
quantile(0.5, x)/quantile(0.99, x). The KLL state column was named after the query's output column, and that name contains the quantile. For example, SQL producedapprox_percentile_cont(lineitem.l_extendedprice,Float64(0.5))and PromQL producedquantile_0_5. CSE compares schemas, so the two producers never matched, even though the quantile is only a readout parameter.What
In
construct_summary_agg(crates/asap-aware-mapping/src/replacement.rs), a Quantile state that reads a single column is now named after that input column. The column is resolved against the child schema, so SQL getsl_extendedpriceand PromQL getsvalue. This follows the existing UnivMon rule that a state's identity does not depend on which statistic reads it. Output column names are unchanged, because the readout node still produces them.Effects:
median(x),approx_median(x)andapprox_percentile_cont(x, 0.5)share one state.TopK is unchanged, because
ksizes the heap and so is a build parameter.Tests
sql_p50_and_p99_share_one_producer: they share, output columns are unchanged, and a differentWHEREor a different column is not shared.cross_series_p50_and_p99_share_one_producer: fails without the fix.value. Their output-column assertions are unchanged.Validation
On Rust 1.99, all of these pass:
cargo fmt --check, clippy with-D warnings, the vendored MetricsQL baseline, andcargo test --workspace.🤖 Generated with Claude Code