Resolve merge conflict in query fuzz test helpers - #7849
Closed
CharlieTLe with Copilot wants to merge 2 commits into
Closed
CharlieTLe with Copilot wants to merge 2 commits into
CharlieTLe with Copilot wants to merge 2 commits into
Conversation
TestVerticalShardingFuzz flakes when promqlsmith emits an expression containing `<lhs> or vector(<rhs>)` against block data with partial time coverage on the LHS: Cortex's vertically-sharded query path and the unsharded reference engine disagree on which timestamps trigger the `vector(...)` fallback, because each shard decides locally whether the LHS is empty at a given step while the unsharded path sees the union. Both answers are individually correct under their own model, but the final results diverge on a handful of points. Same analyser-side family as the known `absent` / `absent_over_time` / `scalar` cases (#5203, #5204, #5205): the Thanos PromQL analyser does not classify `or vector(...)` as non-shardable. Full empirical census in #7547. Add hasOrVectorFallback(parser.Expr), an AST walker that uses parser.Inspect to detect any *parser.BinaryExpr with Op == parser.LOR whose RHS is a *parser.Call to the `vector` function. Call it from isValidQuery alongside the existing `limitk` / `limit_ratio` / `--` filters so every fuzz caller (TestVerticalShardingFuzz plus the seven other fuzz tests that go through runQueryFuzzTestCases) skips the offending shape. An earlier draft used `strings.Contains(queryStr, "vector(")`. Round-2 review flagged that as too broad: a substring scan fires on bare `vector(0)`, `sum(vector(1))`, `vector(1) + on() vector(2)`, and even on incidental `vector(` text inside label literals — none of which diverge between the sharded and unsharded engines. The AST predicate matches only `<lhs> or vector(<rhs>)`, leaving the rest of the random surface intact. Add TestHasOrVectorFallback with six subtests pinning the predicate: `up` (no), `vector(1)` (no — bare call is fine), `up or up` (no — `or` without `vector`), `up or vector(1)` (yes), a deep-LHS positive (`(sum(rate(up[1m])) == bool 0) or vector(0)`), and a simplified form of the actual failing query from #7547. All six pass locally. This is a test-side dodge, not a sharding-engine fix. The proper upstream change — teaching the Thanos analyser to recognise `or vector(...)` as non-shardable, mirroring the existing `absent` / `scalar` exclusions — is tracked separately. Trade-off: isValidQuery is global to all fuzz callers, so random fuzz coverage of `or vector(...)` is dropped everywhere, not just in TestVerticalShardingFuzz. Accepted for the reasons in #7547: the empirical signal points at the sharded vs unsharded path as the only current divergence context, and the filter becomes moot the moment the upstream analyser is fixed. Fixes #7547 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
…ge-conflicts-7551 # Conflicts: # integration/query_fuzz_test.go Co-authored-by: CharlieTLe <3375195+CharlieTLe@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
CharlieTLe
September 18, 2026 18:23
View session
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.
What this PR does:
Resolves the conflict in
integration/query_fuzz_test.goby carrying forward both sides of the change instead of dropping either the PR-specific fuzz exclusion or the newer master-side fuzz helper logic.Conflict resolution
hasOrVectorFallback()guard and its focused unit testFUZZ_SEEDhandling and duplicate-series error canonicalizationResulting behavior
isValidQuery()still rejects the knownor vector(...)false-positive shapemasterMerged shape
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags