DRAFT: Test aggmetrics submetrics - #25180
Draft
kosiew wants to merge 24 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
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.
- 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.
…tion clone - Replace direct `Option` clone with proper handling to avoid unnecessary allocations. - Switch to `datafusion_common::instant::Instant` for more accurate timing measurements. - Update related logic to ensure correctness and performance improvements.
…etric-count regressions - array_agg(DISTINCT) merge: one metric timer/state batch. - convert_to_state: untimed row updates + one conversion timer. - Added metric-count regressions.
…ng, bound prep chunks, time only accumulator calls, and emit one metric update per logical batch - Preps slice/filter outside submetric timing. - Times only accumulator calls. - Bounded 64‑group prep chunks; avoids unbounded retained arrays. - One metric update per logical batch.
github-actions
Bot
added
documentation
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "7a7bd21"
changed:
ref: "1b9cde4" |
Contributor
Author
|
show benchmark queue |
|
Hi @kosiew, you asked to view the benchmark queue (#25180 (comment)).
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 1b9cde4 (1b9cde4) to 7a7bd21 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "7a7bd21"
changed:
ref: "1b9cde4"Results will be posted here when complete File an issue against this benchmark runner |
…pped batch updates and adds regression test **Details:** - Bounded 64‑row prep chunks for `convert_to_state`. - Timer only wraps `update_batch_grouped`. - Excludes factory, slice/filter, `state/evaluate`, and result materialization steps. - Single metric update / logical conversion batch per operation. - Added RED→GREEN filtered multi‑row state‑materialization regression test.
…s, import AggregateMetrics - **Clear conditional metric lookup** – Refactored the conditional logic that retrieves metrics, removing redundant checks and making the code path easier to follow. - **Merged adjacent metrics impl blocks** – Consolidated neighboring implementation blocks that handled similar metric types, reducing duplication and improving maintainability. - **Imported AggregateMetrics; removed qualified repeats** – Added the `AggregateMetrics` import and eliminated unnecessary fully‑qualified references throughout the codebase.
Contributor
Author
|
run benchmark sql env:
CARGO_BUILD_JOBS: 1
BENCH_NAME: array_agg_distinct
BENCH_QUERY: 1
baseline:
ref: "7a7bd21"
changed:
ref: "16a9573" |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 1b9cde4 (1b9cde4) to 7a7bd21 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "7a7bd21"
changed:
ref: "1b9cde4"CPU Details (lscpu)Details
Resource Usagesql — base (merge-base)
sql — branch
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing 16a9573 (16a9573) to 7a7bd21 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "7a7bd21"
changed:
ref: "16a9573"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing 16a9573 (16a9573) to 7a7bd21 diff Run configurationrun benchmark sql
env:
BENCH_NAME: "array_agg_distinct"
BENCH_QUERY: "1"
CARGO_BUILD_JOBS: "1"
baseline:
ref: "7a7bd21"
changed:
ref: "16a9573"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
2 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 submetrics