Repository navigation
fix: an aggregation over a binary expression aggregates the whole binary, like Loki - #741
Merged
Merged
Conversation
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
github-actions
Bot
added
size/XL
Contributor
🟢 Performance A/B: faster, no regression126 shape×range — 1 ✅ fixed · 125 ⚪ same · results vs Loki: 118 same, 0 new difference(s), 8 pre-existing Loki profiles compared (like for like):
125 unchanged shape×range (within noise)
Cold (first-run) timings — what a user opening a panel sees
† Loki's results cache answered this cold run (its own counters, or identical answers for windows a minute apart): not a computed time, so it is in no ratio. ↺ Loki cold taken from the first set that ran the same query; Loki's cache answered this one. 2 shape×range above 2× Loki already (listed, or as slow on the base build)
Why these shapes
base |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
✅ Visual smoke: passed with warningsBase Fix proofCaptures of the exact cases this pull request adds or fixes (registry case, base | PR | Loki). The verdict compares the PR with Loki where the base did not match. The gate fails a case unless one capture is fixed or improved, or, beyond the Loki data window, the base failed where Loki and the PR both answered with data (the label does not excuse it; only
explore-sum-by-over-binary-15m: fixed
explore-sum-by-over-binary-1h: fixed
explore-sum-by-over-binary-6h: undecided
explore-sum-without-comparison-inside-15m: not reproduced on base
explore-sum-without-comparison-inside-1h: fixed
explore-sum-without-comparison-inside-6h: undecided
Regression check
Gate: a base-vs-PR data difference passes when the PR is closer to Loki, and fails when the PR diverges from Loki or Loki cannot decide it (at a range Loki does not hold, a difference passes when the same page proved that query to answer like Loki at a shorter range; otherwise label the pull request dd-fields-1h: history-dependent difference, not gated (1): resource /api/datasources/uid/*/resources/patterns?end=2026-10-29T11%3A34%3
dd-service-logs-1h: history-dependent difference, not gated (1): resource /api/datasources/uid/*/resources/patterns?end=2026-10-29T11%3A34%3
explore-sum-by-over-binary-6h: no Loki data at this range; the same differences match Loki at a shorter range (1): query ('sum by (service_name) (rate(
explore-sum-without-comparison-inside-6h: no Loki data at this range; the same differences match Loki at a shorter range (1): query ('sum without (pod) (rate({ser
lvl-dd-field-pipeline-pill-1h: history-dependent difference, not gated (1): resource /api/datasources/uid/*/resources/patterns?end=2026-10-29T11%3A34%3
Core set montages (3): base | PR | Lokidd-landing-1h
explore-logs-plain-1h
explore-metric-plain-1h
All montages of this run: |
Contributor
PR Quality ReportCompared against base branch Coverage and tests
Compatibility
Performance smokeLower CPU cost (
State
|
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
szibis
force-pushed
the
fix/aggregation-over-binary
branch
from
October 12, 2026 11:45
0257129 to
6df85f0
Compare
github-actions
Bot
added
size/XL
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
szibis
force-pushed
the
fix/aggregation-over-binary
branch
from
October 12, 2026 11:57
6df85f0 to
be827a4
Compare
github-actions
Bot
added
size/XL
szibis
added a commit
that referenced
this pull request
Oct 12, 2026
szibis
force-pushed
the
fix/aggregation-over-binary
branch
from
October 12, 2026 11:59
be827a4 to
5158a0a
Compare
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
… differential pins start+k*step Under a label_replace or label_join the request is cloned to the expression inside it from the request's own query, not from the handler's rewritten one, which may have lost the parser stage an operand needs (a query naming both `| json` and `| logfmt`). The aggregation differential's reference evaluates at start+k*step, as a default Loki does; a join of two vectors off the step grid answers on the aligned grid instead and is registered apart as semantics/binary-step-grid (open). A transform's warnings and statistics are pinned.
… and pin the ignoring shape under an outer sum
The edge, range and differential tables ran on v1.52.0 and v1.40.0; v1.40.0 is
below the supported floor, so they now run on v1.52.0 and v1.49.0, and the
bucket-edge shift that only v1.40.0 needed is gone.
A new edge case pins sum by (job) (A + ignoring(machine) B) with the ignored
label on no series, the shape of semantics/binary-ignoring-matching: Loki joins
both sides on job and the outer sum adds them ({job="a"} 8, {job="b"} 4 on the
fixture); main answers one unlabeled series valued 6. Generated wiring and
parity reports regenerated on the rebased branch.
B1 the visual page's sum by over a rate divided by a literal, B2 a sum by over a ratio of two per-stream metrics, B3 the instant sum(2 * ...) that answered 400, B4 the outer sum over + ignoring(machine) of semantics/binary-ignoring-matching. Generated performance evidence regenerated.
…de-ignored Main opened semantics/without-comparison-inside-ignored while this change was open and pinned the proxy's old answer in TestWithoutShapesStillDifferFromLoki: sum without (pod) (rate(... | unwrap amount [5m]) > 0.33) summed every series, the comparison dropped. A comparison is a binary, so the rebased change sends the aggregation to the binary evaluator, which filters the series before the fold (pkg/logql/evaluator.go:451-466 over vectorBinop). All three pinned cases now equal Loki's answer (range and instant for the rate, instant for max_over_time > 35), and the table failed with "the proxy now answers Loki's". The three cases move to TestWithoutComparisonInsideMatchesLoki, which asserts Loki's answer; TestCompat_AggregationOverBinary gains the sum without over a comparison shape; the row is marked fixed by #741 with a visual fix-proof page.
Every case also asserts that VictoriaLogs was asked no /select/logsql/query: each operand is a stats query and the proxy folds the aggregated series. Checked as well with a probe over sum without (pod) (rate(... | unwrap amount [5m]) > 0.33), sum by over a ratio, the ignoring shape and sum(2 * ...), on both endpoints: one or two stats calls, no raw-row call.
The aggregation-over-binary A/B set (B1-B4) covers semantics/aggregation-over-binary-dropped, and main's conformance gate now requires a perf: block on every covered item. Written by `bench/ab/over2x.py locks seed`: 2.0x Loki max, 1.0x target, cold, warm and refresh, not locked (the daily results of 2026-10-28 have no rows for B1-B4).
…its operands
TestCompat_AggregationOverBinaryMixedParsers failed in e2e-compat (core)
with a 400 "log queries are not supported as an instant query type" from
the proxy for
sum by (route) (sum by (route) (sum_over_time({...js...} | json | unwrap amount [5m]))
/ sum by (route) (count_over_time({...lf...} | logfmt [5m])))
A query naming both | json and | logfmt goes through preferWorkingParser,
which probes the lines and drops the parser that does not fit from the
whole query (removeParserStage). For a metric query that is a text rewrite
that takes a stage's parameters to be everything up to the next `|`; when
the dropped stage was the last one before an operand's range, that was the
rest of the query: the range, the closing parentheses, the operator's other
side. The cut query (`... count_over_time({...lf...}`) was no aggregation
over a binary any more, so /query answered the 400 and /query_range log
streams; a top-level binary of the same operands answered its left operand
alone with a 200 (sum by job 10 and 12 on the unit fixture where Loki
answers 2.5 and 6). Dropping a parser from a binary is wrong in any case:
Loki evaluates each side with its own pipeline (pkg/logql/evaluator.go:385).
preferWorkingParser now leaves a binary, and a vector aggregation over one,
under any label_replace, to its operands (operandsKeepTheirParsers): each
operand is its own handler's query, which is probed on its own, and the
binary reads its operands from the request. A query of one operand naming
both parsers is probed as before (its rewrite is cut the same way; not
covered here). The probe parses the query only when it names both parsers.
TestAggregationOverBinaryMixedParsersAfterTheParserProbe pins Loki's values
for the aggregation, a max over it, a label_replace around it and the
top-level binary, instant and range, on JSON lines (the probe prefers
json); it fails without this. TestParserProbeLeavesBinariesToTheirOperands
pins which queries the probe leaves alone.
This change orders the series of every fold it does by labels.Compare,
as Loki sorts an answer (pkg/logql/engine.go:569 sort.Slice of an instant
vector, :604 sort.Sort of a matrix): the generic without fold
(applyWithoutVector, applyWithoutMatrix) and, through it, the aggregation
over a binary. Main has since put the typed fold (typed_fold.go, foldLevels)
in front of the generic one, and it ordered its groups by their
length-prefixed key text, which is not that order: {x="a"} came before
{job="a",x="a"} (Loki: the other way round, job < x), and a shorter value
before a longer one ({pod="b"} before {pod="ab"}, {pod="api-2"} before
{pod="api-10"}).
The typed fold now sorts its groups with labels.Compare read straight
from the group key, which already holds the kept labels sorted by name as
length-prefixed text, so the sort allocates nothing; the key is unchanged. TestWithoutFoldLabelOrderAndKeys gains
both cases and fails without this.
…pass in the fuzz smoke FuzzFoldBinaryAggregation folds random vectors (labels whose values hold `,`, `=` and quotes; sum, avg, min, max, count; by, without, no grouping, a grouping by an absent label) with foldBinaryAggregation and checks every group's value against a reference of Loki's VectorAggEvaluator.Next and the series order against labels.Compare. It found the typed fold's order (fixed in the previous commit). Values are finite: NaN under min and max is semantics/binary-division-by-zero-nan. FuzzParserProbeLeavesBinaries fuzzes the parameters of the json and logfmt stages (Loki's grammar: names, name="value", commas; a value may hold `|`, `[` and parentheses) of a bare binary, an aggregation over one, a nested aggregation and a label_replace around one, all left to their operands, and of one operand naming both parsers, still probed; any text is read without a panic. Both run in the fuzz-smoke job (12s each).
…ouping label TestAggregationOverBinaryEdgeCases gains, on VictoriaLogs v1.52.0 and v1.49.0 and without reading raw rows: - an operand with no series (its window an hour back holds no line) on the right, on the left, and under count: the join has no series, so the aggregation answers an empty vector (vectorBinop matches nothing, VectorAggEvaluator.Next folds an empty vector to an empty one); - a grouping by a label no series has: one group without labels; - count without every label: one group counting the three series; - max without a label no series has: every series kept.
…label_replace instead of label_join TestCompat_AggregationOverBinaryUnderLabelTransforms failed in e2e-compat (core) on every label_join case with "unhealthy response 400 parse error at line 1, col 1: syntax error: unexpected IDENTIFIER" from Loki (localhost:13101): Loki's grammar has no label_join (pkg/logql/syntax/lex.go maps only label_replace), so there is no Loki answer to compare with. The case is now a label_replace over another label_replace that reads its destination, which Loki answers on every window shape and at an instant. The proxy's own label_join stays covered by the unit tests.
… over a binary The registry row semantics/aggregation-over-binary-dropped and the CHANGELOG entry now describe the parser probe's rewrite of a mixed-parser metric query (it cut the query after the stage it dropped), the typed fold's series order, the label_join e2e cases Loki cannot answer (its grammar has no label_join), the new unit, edge and fuzz tests, and cite Loki 3.7.9 (the same lines as 3.7.7). Wiring regenerated.
…proxies shut down TestAggregationOverBinaryMultiTenant sends `sum(2 * ...)` and `sum by (job) (A / B)` to /query and /query_range with X-Scope-OrgID "0|1" on VictoriaLogs v1.49.0: each tenant is answered as a single tenant is (12; 6 and 6) and the merge labels each series with its __tenant_id__, as for any other metric query (multitenant.go); it is not refused as a log query (multitenant.go has its own instant log-query rejection). The helper behind every aggregation-over-binary handler test now shuts its proxy down.
TestCompat_AggregationOverBinaryMixedParsers now also sends the binary
under the aggregation on its own:
sum by (route) (sum_over_time({...js...} | json | unwrap amount [5m]))
/ sum by (route) (count_over_time({...lf...} | logfmt [5m]))
The parser probe rewrote it like the aggregation (it cut the query after
the json operand) and the proxy answered the left operand alone with a 200.
…join differential's
…s after the rebase
Collaborator
Author
|
Verification for head
Required review is the remaining merge gate. |
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.
Problem and result
Loki evaluates a binary expression before an outer
sum,avg,min,max, orcount. The proxy previously dropped the binary operator and sometimes its right operand, returning HTTP 200 with wrong values. For example,sum(rate({app="x"}[1m]) / 2)returned the undivided rate. This PR evaluates both operands, applies Loki-compatible vector matching, then folds the joined series with the requested grouping on instant and range queries.The branch is rebased on main through #918. It also keeps parser stages local to each operand, preserves Loki's label order, handles
label_replacearound the aggregation, and fixes namedlogfmtextraction so unrelated parsed fields do not leak into series labels. Narrow rewrites forsum(rate(selector)/scalar)andsum(rate(selector)/count_over_time(selector))avoid per-stream backend reads in the B1/B2 performance shapes. The README one-glance comparison and linked performance docs now use the saved 2026-10-29 daily A/B; the June benchmark remains clearly historical.Visual smoke exposed a 6-hour
sum without (pod) (rate(... | logfmt amount | unwrap amount [20s]) > 40)panel error: the per-stream operand crossed the 64 MiB manual response guard before the outer sum reduced it. The proxy now retries that specific oversized response on disjoint evaluation-step ranges and merges the folded series, preserving the existing response limit.Verification
go test -count=1 ./internal/proxypassed on166837e8(371.7 s);./internal/logqland./internal/translatorpassed on the earlier functional head.python3 scripts/ci/check_conformance.py,python3 scripts/ci/check_changelog_pr.py --base f91dbc37 --head HEAD, andgit diff --checkpassed on this head.166837e8: CI passed its race/coverage suite, E2E/UI/compatibility matrix, stress, fuzz, lint, build, and benchmark jobs. Visual smoke passed with proof that the two fixed Explore pages match Loki at 1h and the 6hsum withoutpanel no longer errors. Performance A/B passed 126 shape×range checks with 0 new Loki differences. Quality report reports 18,723 tests (+66), 89.0% coverage, and 100% on its pinned Loki, Drilldown, and VictoriaLogs compatibility tracks. All 67 PR checks pass; GitHub reports no conflicts. Required review remains the merge gate.Known open differences remain in the conformance registry and generated performance report; this PR does not claim to close them. No release or merge is requested here.