Test aggmetrics - #25096
Draft
kosiew wants to merge 17 commits into
Draft
Conversation
- Added harness implementation: - `benchmarks/sql_benchmarks/array_agg_distinct/array_agg_distinct.suite` – defines the benchmark suite, test parameters, and execution configuration for the data‑free SQL harness. - `benchmarks/sql_benchmarks/array_agg_distinct/benchmarks/q01.benchmark` – contains the specific query benchmark (`q01`) that exercises the `array_agg(DISTINCT …)` workload without requiring any input data. - Workload characteristics: - Simulates **2 M range rows** → **1 M groups**. - Each group contains **2 rows** with **2 distinct values**, providing a realistic yet data‑free test scenario for aggregation performance. - add bench.sh wrapper for array_agg_distinct
kosiew
force-pushed
the
test-aggmetrics
branch
from
September 9, 2026 04:33
ddfe85c to
372b160
Compare
github-actions
Bot
added
documentation
Contributor
Author
|
run benchmark array_agg_distinct baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 372b160 (372b160) to b827774 diff Run configurationrun benchmark array_agg_distinct
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7"Results will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25096 +/- ##
==========================================
+ Coverage 81.72% 81.81% +0.08%
==========================================
Files 1127 1130 +3
Lines 416519 418129 +1610
Branches 416519 418129 +1610
==========================================
+ Hits 340401 342079 +1678
+ Misses 56115 55900 -215
- Partials 20003 20150 +147 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
show benchmark queue |
|
Hi @kosiew, you asked to view the benchmark queue (#25096 (comment)). No pending jobs. File an issue against this benchmark runner |
Contributor
Author
|
run benchmark array_agg_distinct env:
CARGO_BUILD_JOBS: 1
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 372b160 (372b160) to b827774 diff Run configurationrun benchmark array_agg_distinct
env:
CARGO_BUILD_JOBS: "1"
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7"Results will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 372b160 (372b160) to b827774 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7"Results will be posted here when complete File an issue against this benchmark runner |
Contributor
Author
|
show benchmark queue |
|
Hi @kosiew, you asked to view the benchmark queue (#25096 (comment)).
File an issue against this benchmark runner |
|
Benchmark for this request hit the 7200s job deadline before finishing. Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
Contributor
Author
|
run benchmark sql env:
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 372b160 (372b160) to b827774 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
baseline:
ref: "b827774378aedcb73b921d3b44ccb6491b595fc2"
changed:
ref: "372b16099fe83eaa62cd27dcd7435ee3f6dc6fd7"Results will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
kosiew
force-pushed
the
test-aggmetrics
branch
from
September 9, 2026 08:54
372b160 to
908b5ef
Compare
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "09d65873070ed225737ed22bf47744a6712d6715"
changed:
ref: "908b5ef76cfde9e75dff8733f685246c61984b30" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 908b5ef (908b5ef) to 09d6587 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "09d65873070ed225737ed22bf47744a6712d6715"
changed:
ref: "908b5ef76cfde9e75dff8733f685246c61984b30"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 908b5ef (908b5ef) to 09d6587 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "09d65873070ed225737ed22bf47744a6712d6715"
changed:
ref: "908b5ef76cfde9e75dff8733f685246c61984b30"CPU Details (lscpu)Details
Resource Usagesql — base (merge-base)
sql — branch
File an issue against this benchmark runner |
Reduce the data-free grouped array_agg(DISTINCT) workload while preserving its two-rows-per-group and two-distinct-values-per-group shape.
…comprehensive tests
- Introduce optional `AggregateMetric(s)` API with default no‑op setters for backward compatibility.
- Add lazy‑stable internal metrics: `agg_expr_{i}_internal_{subphase}_time` for fine‑grained phase tracking.
- Wire the metrics across all execution paths: stream, grouped, hash, ordered, and replay.
- Implement `array_agg(DISTINCT)` distinct‑timer to measure distinct‑aggregation latency.
- Extend test coverage:
- Partition merge scenarios.
- Repeated DISTINCT expression handling.
- Update documentation to reflect new API, metric naming, and wiring details.
…ove performance Cache one adapter per subphase; no repeated wrapper allocation - Introduce a single reusable adapter instance per subphase, eliminating the need to allocate multiple wrapper objects. This reduces memory churn and improves performance during metric collection. Skip `Arc` clone/clock read when no metric - Detect when there is no active metric to record and skip the unnecessary `Arc` clone and system clock reads. This lowers CPU overhead for subphases that don't emit metrics. Make submetric implementation details private - Move internal helpers and type-specific logic for submetrics behind `pub(super)` or module‑level privacy boundaries. This hides implementation details from external users, enhancing encapsulation and reducing the risk of misuse.
- Updated the test to verify that both `internal_distinct` timers are positive (`>0`) - This resolves the blocker where timers could be zero, causing test failures - Ensures correct initialization and behavior of the timer logic - Improves the reliability and confidence of timer‑related functionality
… partition display, and empty input behavior - Clarify identity/cardinality format as **(expr index, subphase, partition)** - Note that replacement accumulators share a single timer - Explain how partitions are combined in the normal display - Document that construction‑time requests can cause metrics to appear on empty input
- Counts the retained metric‑handle field in the accumulator’s size. - Adds a regression test to verify the size calculation under various inputs. - Updates the exact distinct‑size expectation to match the corrected behavior.
…stinct identities and timer handling - Add new 2‑partition execution test for `aggregate_stream.rs`. - Fix repeated `array_agg(DISTINCT ...)` identities and labels to be consistent. - Ensure exact partition identity handling: 0/1 per repeated expression. - Merge timer total calculation into a single metric. - Assert that normal `sum` operations do not produce any internal metric.
- Implement a lock‑free `OnceLock` fast path for the first/common subphase in `AggregateSubMetrics`, eliminating per‑group `Mutex` and `HashMap` lookups for distinct subphases. - Keep the locked map only for handling extra / uncommon subphases, reducing contention and improving performance. - Preserve zero‑duration timers internally, preventing the 1 ns‑per‑call inflation that previously affected fast paths. - Ensure time‑merging logic respects zero‑duration metrics, avoiding spurious non‑zero contributions. - Add comprehensive tests covering caching behavior, zero‑duration scenarios, and multi‑subphase usage.
- Restored shared Time::add min‑1ns behavior. - Removed exact‑duration API/use. - Added regression test merge‑zero → recorded 1ns. - Updated submetric zero test.
…per‑batch 1 ns inflation - Time::add: legacy min‑1 ns merge unchanged. - Time::add_duration_exact: restored, scoped API. - Aggregate submetrics use exact adds → no per‑batch 1 ns inflation. - Tests cover both contracts.
…ile‑time test for DistinctArrayAggAccumulator - Updated `AggregateMetric` to implement the `RefUnwindSafe` trait, ensuring safe reference semantics during unwind operations. - Added a compile‑time test (`distinct_array_agg_accumulator_unwindsafe`) that verifies `DistinctArrayAggAccumulator` maintains both `UnwindSafe` and `RefUnwindSafe` guarantees, preventing panics in error‑recovery scenarios.
kosiew
force-pushed
the
test-aggmetrics
branch
from
September 9, 2026 10:46
908b5ef to
e3c11e7
Compare
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "e3c11e7a266ae9956f9c7c3e007d13301279522a" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing e3c11e7 (e3c11e7) to 90b6f65 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "e3c11e7a266ae9956f9c7c3e007d13301279522a"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing e3c11e7 (e3c11e7) to 90b6f65 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "e3c11e7a266ae9956f9c7c3e007d13301279522a"CPU Details (lscpu)Details
Resource Usagesql — base (merge-base)
sql — branch
File an issue against this benchmark runner |
…mprove per-group metrics - Skip internal DISTINCT timing for batches <16 to reduce overhead. - Avoid per-group `Instant::now()`, metric `Arc` clone, and atomic updates for small batches. - Metrics are still recorded for batches >=16 to retain visibility where needed. - Added threshold tests to verify the new behavior.
- Direct `GroupedHashAggregateStream` usage. - Multiple groups → adapter per‑group accumulators. - Asserts positive `agg_expr_0_internal_distinct_time`.
…mprove per-group metrics - Skip internal DISTINCT timing for batches <16 to reduce overhead. - Avoid per-group `Instant::now()`, metric `Arc` clone, and atomic updates for small batches. - Metrics are still recorded for batches >=16 to retain visibility where needed. - Added threshold tests to verify the new behavior.
…for accumulators This change introduces two new methods to the Accumulator trait: 1. `grouped_update_batch_metric`: Returns an optional metric that can be used to time grouped updates once per batch instead of per group. 2. `update_batch_grouped`: Updates state when called by a grouped accumulator adapter, with support for using the grouped update metric. The GroupsAccumulatorAdapter now uses these new methods to avoid timing every per-group call, recording one interval for the full batch instead. This reduces the overhead of metric collection when there are many groups. DistinctArrayAggAccumulator is updated to take advantage of the new grouped update functionality, skipping per-group timing for deduplication operations. This refactor improves performance for grouped aggregations by making metric collection proportional to batch count rather than group cardinality.
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "fe1ca2432935210f71da16357e74787697e372b4" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fe1ca24 (fe1ca24) to 90b6f65 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "fe1ca2432935210f71da16357e74787697e372b4"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fe1ca24 (fe1ca24) to 90b6f65 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "90b6f659b09d73049f92c01e4157e3a249fcce3e"
changed:
ref: "fe1ca2432935210f71da16357e74787697e372b4"CPU Details (lscpu)Details
Resource Usagesql — base (merge-base)
sql — branch
File an issue against this benchmark runner |
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
3 participants
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.
test aggmetrics benchmark requested in this comment