Add projection pushdown to binary expression#691
Conversation
Signed-off-by: yeya24 <benye@amazon.com>
96bd215 to
90d332a
Compare
Signed-off-by: yeya24 <benye@amazon.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the existing projection optimizer so it can push projection requirements down into binary expressions (specifically many-to-one / one-to-many joins), allowing the binary vector operator to avoid materializing unnecessary labels when building join-table result metrics—reducing memory usage for high-cardinality joins.
Changes:
- Add optional
PushDownBinaryProjectionmode toProjectionOptimizerto store projections onBinarynodes and derive/push child projections. - Plumb
Binary.Projectionthrough logical nodes, execution planning, and into the binary vector operator to apply label filtering duringresultMetric()materialization. - Add/expand unit tests, correctness tests (including Prometheus comparison), and add benchmarks for binary projection pushdown.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| logicalplan/projection.go | Adds PushDownBinaryProjection option and stores projection on Binary nodes for group_left/right cases. |
| logicalplan/logical_nodes.go | Extends Binary node with Projection and includes it in cloning + JSON marshal/unmarshal. |
| execution/execution.go | Passes binary projection from logical plan into the execution binary operator. |
| execution/binary/vector.go | Applies projection during binary resultMetric() label materialization to reduce label allocations. |
| logicalplan/plan_test.go | Updates plan rendering to display binary projections in test output. |
| logicalplan/projection_test.go | Updates projection expectations and adds targeted tests for binary projection pushdown behavior. |
| engine/projection_test.go | Adds correctness tests for pushdown (baseline vs optimized, and Prometheus comparison) and fuzz-like coverage focused on binaries with projections. |
| engine/projection_binary_bench_test.go | Adds benchmarks for measuring memory/CPU impact of binary projection pushdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| testutil.Ok(b, err) | ||
| series, _ := op.Series(context.Background()) | ||
| if i == 0 { | ||
| b.Logf("Result series count: %d", len(series)) |
There was a problem hiding this comment.
This benchmark ignores the error return from op.Series(...). Even in benchmarks, it’s useful to check and fail fast so we don’t end up measuring behavior under error conditions silently.
| qBaseline, err := engineBaseline.NewRangeQuery(context.Background(), storage, nil, tc.query, start, end, interval) | ||
| testutil.Ok(t, err) | ||
| resultBaseline := qBaseline.Exec(context.Background()) | ||
| testutil.Ok(t, resultBaseline.Err) |
There was a problem hiding this comment.
Range queries created in this test aren't closed. The engine tests run with goleak verification, so leaving queries open can leak goroutines/resources and make the suite flaky. Please add defer qBaseline.Close() after successful creation (and similarly close the optimized query).
| qOptimized, err := engineOptimized.NewRangeQuery(context.Background(), storage, nil, tc.query, start, end, interval) | ||
| testutil.Ok(t, err) | ||
| resultOptimized := qOptimized.Exec(context.Background()) | ||
| testutil.Ok(t, resultOptimized.Err) |
There was a problem hiding this comment.
Range query qOptimized is not closed after Exec. Please defer qOptimized.Close() after creation to avoid leaking resources/goroutines under goleak.
| testutil.Ok(t, err) | ||
| resultProm := qProm.Exec(context.Background()) | ||
| testutil.Ok(t, resultProm.Err) | ||
|
|
||
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | ||
| testutil.Ok(t, err) |
There was a problem hiding this comment.
Prometheus range query qProm is never closed. Please defer qProm.Close() (and likewise for qThanos) to avoid leaking goroutines/resources during the engine_test goleak-verified test suite.
| testutil.Ok(t, err) | |
| resultProm := qProm.Exec(context.Background()) | |
| testutil.Ok(t, resultProm.Err) | |
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | |
| testutil.Ok(t, err) | |
| testutil.Ok(t, err) | |
| defer qProm.Close() | |
| resultProm := qProm.Exec(context.Background()) | |
| testutil.Ok(t, resultProm.Err) | |
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | |
| testutil.Ok(t, err) | |
| defer qThanos.Close() |
| testutil.Ok(t, err) | ||
| resultProm := qProm.Exec(context.Background()) | ||
| testutil.Ok(t, resultProm.Err) | ||
|
|
||
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | ||
| testutil.Ok(t, err) |
There was a problem hiding this comment.
Thanos range query qThanos isn't closed. Add defer qThanos.Close() after creation to avoid goroutine/resource leaks under goleak.
| testutil.Ok(t, err) | |
| resultProm := qProm.Exec(context.Background()) | |
| testutil.Ok(t, resultProm.Err) | |
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | |
| testutil.Ok(t, err) | |
| testutil.Ok(t, err) | |
| defer qProm.Close() | |
| resultProm := qProm.Exec(context.Background()) | |
| testutil.Ok(t, resultProm.Err) | |
| qThanos, err := thanosEngine.NewRangeQuery(context.Background(), storage, nil, query, start, end, interval) | |
| testutil.Ok(t, err) | |
| defer qThanos.Close() |
| expected string | ||
| }{ | ||
| { | ||
| name: "aggregation with binary using on - binary gets projection", |
There was a problem hiding this comment.
This test case name says the binary gets a projection, but for * on(job) the vector matching cardinality is one-to-one, and the optimizer intentionally does not set Binary.Projection in that case. Consider renaming the case to reflect that only the selectors get projections here (or adjust the expectation if the intent changed).
| name: "aggregation with binary using on - binary gets projection", | |
| name: "aggregation with binary using on - selectors get projections", |
| // Store projection on Binary only when the binary has group_left or group_right. | ||
| // For one-to-one or vector-scalar, projecting the binary's output can collapse distinct | ||
| // series to the same label set and cause implicit many-to-one in a downstream binary. | ||
| if p.PushDownBinaryProjection && projection != nil && n.VectorMatching != nil && | ||
| (n.VectorMatching.Card == parser.CardManyToOne || n.VectorMatching.Card == parser.CardOneToMany) { | ||
| n.Projection = &Projection{ | ||
| Labels: append([]string(nil), projection.Labels...), | ||
| Include: projection.Include, | ||
| } | ||
| } |
There was a problem hiding this comment.
pushProjection currently stores a Projection on Binary whenever projection != nil and the binary is many-to-one/one-to-many. If the incoming projection is a no-op (e.g. exclude mode with an empty label list, such as sum without ()), this unnecessarily sets Binary.Projection and can add noise/overhead (and affects renderers/tests). Consider only setting n.Projection when it will actually change the output labels (e.g. projection.Include || len(projection.Labels) > 0).
| if t.Projection != nil { | ||
| b.WriteString("(") | ||
| } | ||
| b.WriteString(renderExprTree(t.LHS)) | ||
| b.WriteString(" ") | ||
| b.WriteString(t.Op.String()) |
There was a problem hiding this comment.
renderExprTree wraps every Binary with Projection != nil in parentheses even if no projection suffix will be printed (e.g. exclude mode with 0 labels). This can make rendered output misleading and can introduce unnecessary diffs in test expectations. Consider only adding parentheses/suffix when the projection is effective (Include=true or len(Labels)>0).
Fixes #689
The idea is to reuse the same projection logical optimizer to pushdown projections to binary expression. Binary expression vector operator can use the projection information to skip non projected labels when materializing labels in the join table.
Added comprehensive tests and correctness tests to ensure the correctness.
My local benchmark showed that this helps mainly when label string interning is disabled. We are still using
slicelabelsin Cortex so it helps with our usecase. Users can choose whether to enable or disable this functionality.Here is the AI generated benchmark report based on the benchmarks I ran locally.
Binary Projection Pushdown - Benchmark Results
Test Configuration
-tags slicelabels(label interning disabled)Results
Small Dataset (1K series, 10 labels)
Without Projection:
With Projection:
Savings:
Large Dataset (10K series, 20 labels)
Without Projection:
With Projection:
Savings:
Key Findings
Scaling Behavior
The optimization's benefits scale linearly with dataset size:
Why It Works (with slicelabels)
Memory Breakdown (Large Dataset)
Without projection (22.3 MB):
With projection (4.1 MB):
Savings: 18.2 MB (82%)
Impact of Label Interning
With Default Build (Label Interning Enabled)
The optimization provides minimal benefit because:
With slicelabels Build (Label Interning Disabled)
The optimization provides massive benefit because:
Real-World Implications
For Prometheus (Default Build with Interning)
Not recommended - The optimization adds CPU overhead without meaningful memory savings.
For Systems Without Label Interning
Highly recommended - Provides:
When to Enable
Enable this optimization when:
Conclusion
The optimization is correctly implemented and provides dramatic benefits for systems without label interning: