iframe-proxy

pugachevdmitry · 2026-03-31T09:53:27Z

Adds the -Sparkbar combinator that applies any numeric-result aggregate function independently to each x-axis bucket and renders the per-bucket results as a Unicode sparkbar string.

-- Count rows per date bucket
SELECT countSparkbar(20, toDate('2022-01-01'), toDate('2024-02-10'))(Date)
FROM table;

-- Sum values per date bucket
SELECT sumSparkbar(20, toDate('2022-01-01'), toDate('2024-02-10'))(Date, value)
FROM table;

-- Unique visitors per date bucket
SELECT uniqSparkbar(20, toDate('2022-01-01'), toDate('2024-02-10'))(Date, ClientIP)
FROM table;

The combinator works with any aggregate function that returns a numeric type. The first argument is the x-axis (bucket key) column; remaining arguments are forwarded to the nested function. An explicit range [begin_x, end_x] is required because bucketing happens at aggregation time.

Supported key column types: UInt8–UInt64, Int8–Int64, Date, Date32, DateTime, DateTime64, Enum, Interval.

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Added the -Sparkbar aggregate function combinator. It applies numeric-result aggregate functions such as sum, count, and uniq to values grouped into x-axis buckets and renders the results as a Unicode sparkbar string. Example: sumSparkbar(20, toDate('2022-01-01'), toDate('2024-12-31'))(Date, value).

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

The combinator follows the same parameter convention as the existing sparkbar function: {fn}Sparkbar(width, begin_x, end_x)(x_col, ...). Documentation should be added to docs/en/sql-reference/aggregate-functions/combinators.md.

Note

Medium Risk
Adds a new aggregate combinator that changes aggregation state layout and parameter/type validation, which could impact query correctness/performance for affected functions. Risk is mitigated by strict argument checks and dedicated stateless tests across key types (including DateTime64 scaling).

Overview
Adds a new -Sparkbar aggregate function combinator that buckets rows by a first “x-axis” argument over an explicit [min_x, max_x] range, runs the nested aggregate per bucket, and returns a Unicode sparkbar String.

The implementation registers the new combinator, enforces numeric nested-result types and supported key types (including DateTime64 scale alignment), and introduces stateless tests plus user docs describing syntax, parameters, and examples.

^{Reviewed by Cursor Bugbot for commit 8ae26e0. Bugbot is set up for automated code reviews on this repo. Configure here.}

CLAassistant · 2026-03-31T09:53:39Z

clickhouse-gh · 2026-03-31T10:01:37Z

Workflow [PR], commit [46800c2]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Stress test (arm_release)		FAIL
	Logical error: Invalid number of rows in Chunk A column B at position C: expected D, got E (STID: 3366-33da)	FAIL	cidb
Upgrade check (amd_release)		FAIL
	Error message in clickhouse-server.log (see upgrade_error_messages.txt)	FAIL	cidb

AI Review

Summary

This PR adds the -Sparkbar aggregate function combinator with documentation and focused stateless coverage for forwarded nested parameters, DateTime64 scale handling, exact 64-bit bucket indexing, non-positive rendering semantics, and cross-variant state merging. I rechecked the current code against the prior review threads and did not find remaining correctness or safety issues that should block merge.

Final Verdict

Status: ✅ Approve

Introduces the `-Sparkbar` combinator that applies any aggregate function independently to each x-axis bucket and renders the per-bucket results as a Unicode sparkbar string. Usage: sumSparkbar(width, begin_x, end_x)(x_col, value_col) countSparkbar(width, begin_x, end_x)(x_col) uniqSparkbar(width, begin_x, end_x)(x_col, key_col) The first argument is the x-axis (bucket key) column; remaining arguments are forwarded to the nested aggregate function. The explicit range [begin_x, end_x] is required because bucketing must happen at aggregation time before the per-bucket states are accumulated. Supported key column types: UInt8–UInt64, Int8–Int64, Date, Date32, DateTime, DateTime64, Enum, Interval.

…e docs

…ions - Fix error test annotations: wrong parameter count gives `NUMBER_OF_ARGUMENTS_DOESNT_MATCH` (42), not `SYNTAX_ERROR` - Remove empty-parens test case for "no key argument" since `f(params)()` is rejected by the parser with `SYNTAX_ERROR` before reaching our code - Replace non-triggerable nested-type test with unsupported x-axis type test - Add `.reference` file generated via `clickhouse-test --record` - Remove unused `BAD_ARGUMENTS` declaration from `.cpp` (it is used in `.h`)

The error text now lists all accepted key types: native integer, Date, Date32, DateTime, DateTime64, Enum, and Interval.

Strip Nullable wrapper with removeNullable before the isNumber check, so compositions like avgOrNullSparkbar are accepted.

Compute integer deltas before Float64 conversion to avoid catastrophic cancellation for large Int64/DateTime64 key values near ±1e18. Update documentation: the result can be an empty string when all buckets are empty or zero, rather than a fixed-width space-padded string.

DateTime64 literals are stored as Decimal64 fields, not UInt64. Move DateTime64 into its own branch and extract tick count via FieldVisitorConvertToNumber<Int64>. Fix "Let's you" -> "Lets you" in -Resample and -Sparkbar docs sections.

…binator columns are backed by , which implements but not . The previous branch for unsigned types called , causing a NOT_IMPLEMENTED exception at runtime. Fix: always read the x-axis key via and cast to . This is safe for all supported types: - (Date, DateTime, native UInt*): tick values fit in Int64 - / : is the natural accessor - : underlying stores the tick count as Int64 Also add a functional test covering x-axis parameters and update the file accordingly.

…tion DateTime64 parameters are stored in Field as DecimalField<DateTime64>, where getValue() returns the raw tick count. The previous code used FieldVisitorConvertToNumber<Int64>, which divides by the scale multiplier (e.g. 1000 for scale=3), turning ticks into seconds. In add(), getInt returns raw ticks, so begin_x/end_x and key were in different units and almost all rows were dropped. Fix: extract parameters via safeGet<DecimalField<DateTime64>>().getValue() to preserve raw ticks regardless of scale.

Pre-epoch DateTime64 timestamps have negative raw tick counts. The previous code used UInt64 as Key for DateTime64, so negative begin_x/end_x were cast to large unsigned values, causing the begin_x >= end_x constructor check to throw BAD_ARGUMENTS for valid cross-epoch ranges. Fix: use Int64 as Key for DateTime64 (same as Int*), keeping tick counts signed end-to-end. The UInt64 subtraction trick in add() is unaffected: after the key >= begin_x bounds check, both deltas are mathematically non-negative, so two's complement wraparound gives correct results. Add a cross-epoch test (begin_x=-2000ms, end_x=+2000ms) to prevent regression.

clickhouse-gh · 2026-06-04T14:24:32Z

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

no significant changes detected. K_source=6 K_base=30 flagged=0/65

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info

StressHouse run: 0f15e1ed-0cf7-426e-aa2c-bab0873169a8
MIRAI run: c5395c0c-7aac-44f0-800b-3200007fc8ab
PR check IDs:
- clickbench_335596_1782687864
- clickbench_335610_1782687864
- clickbench_335616_1782687864
- tpch_adapted_1_official_335623_1782687864
- tpch_adapted_1_official_335633_1782687864
- tpch_adapted_1_official_335646_1782687864

Initialize the temporary `tmp` variable in `transformAggregateFunction` so the `cppcoreguidelines-init-variables` clang-tidy check passes (the `arm_tidy` build treated it as an error). Also mirror `AggregateFunctionResample` by overriding `canMergeStateFromDifferentVariant` and `mergeStateFromDifferentVariant`, delegating per bucket to the nested function. Without this, merging a sparkbar state produced in a different aggregation/window variant of a nested function that supports cross-variant merge (such as `cramersV` or `theilsU`) would be rejected before any bucket could be merged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-06T15:50:47Z

Picked up this PR via /continue-pr:

Merged master (the branch was ~750 commits behind and red); no conflicts. Renumbered the test 04071_sparkbar_combinator → 04322_sparkbar_combinator because 04071 now collides with several tests added on master.
Fixed the arm_tidy CI failure: the cppcoreguidelines-init-variables check (treated as an error) flagged the uninitialized tmp in transformAggregateFunction. Initialized it to 0.
Addressed the bot review thread on cross-variant merge: the combinator now mirrors AggregateFunctionResample, overriding canMergeStateFromDifferentVariant and mergeStateFromDifferentVariant so a nested function that supports cross-variant merge (e.g. cramersV, theilsU) can have its sparkbar states merged bucket-by-bucket.

Verified locally on an ARM build: the object compiles clean, the full test 04322_sparkbar_combinator matches its reference, the four serverError cases return the expected codes (42 / 36 / 43 / 407), and cramersVSparkbar runs.

The DateTime64 branch rescaled the `begin_x`/`end_x` parameters down to the column scale, rounding `begin_x` up and `end_x` down. When the column scale was coarser than the parameter scale, a valid fine-scale range could collapse to `begin_x >= end_x`, making the constructor throw `BAD_ARGUMENTS` even though the range contained a representable column tick (and a truly sub-tick range threw instead of producing an empty bar). Bucket entirely at the finest scale among the column and the two bounds instead. All three quantities are then expressed in the same unit and obtained by exact multiplication (never division or rounding), so a range valid at fine scale can never collapse, single-tick ranges are aggregated, and sub-tick ranges render an empty bar. Keys read from the column are rescaled up to the working scale in `add` via a per-instance multiplier (1 for every non-DateTime64 key type). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-08T10:02:55Z

Picked this up to advance it:

Merged master (the branch was ~422 commits behind). This also pulls in Fix abort on malformed uniqTheta state in ThetaSketchData::read #106260, which fixed the uniqTheta std::out_of_range abort in ThetaSketchData::read — that was the sole CI failure (the AST fuzzer crash, issue LOGICAL_ERROR Code 1001 (SIGABRT) on invalid AggregateFunction(uniqTheta, ...) state — std::exception from datasketches escapes SerializationAggregateFunction's catch block #106259, now closed) and was unrelated to this PR.
Fixed the open Bugbot review thread about DateTime64 range collapse (0ee7b05153f): the DateTime64 branch used to rescale the bounds down to the column scale with directional rounding, which could collapse a valid fine-scale range to begin_x >= end_x and throw BAD_ARGUMENTS. Bucketing now happens at the finest scale among the column and the two bounds, using exact multiplication only. Single-representable-tick ranges are aggregated and sub-tick ranges render an empty bar instead of throwing. Added regression tests; all existing reference outputs are unchanged.

Built cleanly and 04322_sparkbar_combinator passes locally.

The existing 04322_sparkbar_combinator test never exercises the cross-variant merge path (mergeStateFromDifferentVariant) that -Sparkbar implements. A nested-function state produced in a window context (...SparkbarState(...) OVER ()) stores a different aggregate function variant than the one used by ...SparkbarMerge, so finalizing it must go through mergeStateFromDifferentVariant for every per-bucket sub-state. Add 04329_sparkbar_combinator_state_variant, mirroring 04079_crosstab_window_state_variant_cast.sql, using cramersV as the nested function (it has a distinct window variant). The structural (width, begin_x, end_x) parameters are repeated on -Merge, as required for a combinator whose state layout depends on them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address review: the per-bucket index was computed in `Float64`, which loses precision once the range or the offset exceeds the 53-bit exact integer range, silently mis-bucketing valid `UInt64`/`Int64`/`DateTime64` keys near a bucket boundary. For example `countSparkbar(2, 0, 9007199254753337)` placed the key `4503599627376668` in bucket 1, although exact arithmetic puts it in bucket 0 (`2 * offset = range - 1 < range`), because `Float64(9007199254753337)` rounds to its nearest even double `9007199254753336`. Compute the bucket index with exact integer arithmetic instead. The deltas are already taken in `UInt64` (so the sign of `begin_x`/`end_x` and large magnitudes are handled by modular subtraction); the `offset * width` product is taken in 128 bits because it can exceed `UInt64`, then divided by the range. The quotient is at most `width` (reached only when `key == end_x`) and is clamped to the last bucket, matching the previous `std::min` clamp. Bucket assignments are unchanged for every existing test (all ranges are below 2^53); only the >2^53 boundary cases change, and they now match exact integer arithmetic. Add `04330_sparkbar_combinator_large_range_bucketing` covering `Int64` and `UInt64` keys at the exact bucket boundary of a >2^53 range, so reverting to a `Float64` index computation fails the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

After merging `master`, the test number 04330 collides with several existing tests (`04330_arrow_oob_declared_length`, `04330_show_policies_format_clause`, `04330_tuple_element_aggregation_disabled_by_default`). Renumber to 04342, the next free prefix, to keep test numbers unique. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`master` added a `Documentation` parameter to `registerCombinator` and gave every combinator an embedded description and syntax that are exposed through the new `system.aggregate_function_combinators` table. After merging `master`, `-Sparkbar` was the only combinator left without it. Add documentation matching that convention, mirroring the `-Resample` combinator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-17T03:27:40Z

Addressed the AI review "Request changes" verdict (exact 64-bit bucket indexing) and refreshed the branch against master:

370bfcb9d74 — exact integer bucket index. Replaced the Float64 bucket-position computation with pos = static_cast<UInt64>((__uint128_t)offset * width / range), clamped to the last bucket. This removes the precision loss for range/offset beyond 2^53 (e.g. DateTime64(9) over a >104-day span, or large UInt64/Int64 ranges). Bucket assignments are provably unchanged for every existing test (all ranges < 2^53). Added 04342_sparkbar_combinator_large_range_bucketing so reverting to Float64 fails the test.
Merged master (the branch was ~8 days / 2636 commits behind), clean, no conflicts. Renumbered the new regression test off 04330 (now taken on master) to 04342.
314348aa6a3 — embedded combinator documentation. master added a Documentation argument to registerCombinator (exposed via system.aggregate_function_combinators); -Sparkbar was the only combinator left without it, so I added a description and syntax mirroring -Resample.

Verified the changed combinator TU compiles against merged master (-fsyntax-only, clean). A full local build/test run was not possible due to disk constraints, so CI will validate the build and the new test.

The embedded `system.aggregate_function_combinators.syntax` string for `-Sparkbar` documented only `Sparkbar(width, begin_x, end_x)`, omitting the optional nested aggregate parameters that the implementation forwards (e.g. the quantile level in `quantileSparkbar(0.9, width, begin_x, end_x)`). The Markdown docs already describe this optional prefix, so generated/system-table help pointed users to a form that cannot express parametric nested aggregates. Add the optional forwarded-parameter prefix to the embedded syntax so it matches the documented feature and `docs/.../combinators.md`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

After merging `master`, the test prefixes `04322` and `04329` collided with several tests added on `master` (e.g. `04322_limit_by_monotonic_desc_key`, `04329_or_join_packed_key_merge`). Renumber the two colliding sparkbar tests to free prefixes: - `04322_sparkbar_combinator` -> `04344_sparkbar_combinator` - `04329_sparkbar_combinator_state_variant` -> `04345_sparkbar_combinator_state_variant` `04342_sparkbar_combinator_large_range_bucketing` was already unique and is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-21T03:23:59Z

Picked this up via /continue-pr to refresh the branch and analyze the red CI.

Merged master (the branch was 1269 commits behind and red). The merge is clean with no conflicts. The net diff vs master is purely additive — the new AggregateFunctionSparkbar.{h,cpp} files, the docs, the 2-line registration in registerAggregateFunctions.cpp, and the three tests. All aggregate-combinator interface headers the code depends on (IAggregateFunction.h, IAggregateFunctionCombinator.h, AggregateFunctionCombinatorFactory.h, ISerialization.h) are byte-identical to the merge-base, so no adaptation was needed.

Renumbered two colliding tests. After the merge, the prefixes 04322 and 04329 collided with tests added on master:

04322_sparkbar_combinator → 04344_sparkbar_combinator
04329_sparkbar_combinator_state_variant → 04345_sparkbar_combinator_state_variant

04342_sparkbar_combinator_large_range_bucketing was already unique and is unchanged. The renames are pure (the files contain no internal references to their numbers).

CI analysis — all failures are pre-existing and unrelated to this PR. This PR only adds an isolated new aggregate combinator (plus one registration line), and none of the failures touch that path:

Stress test (amd_debug) → Logical error: 'Stream v.variant_discr for column v with type Dynamic(max_types=5) is not found' in MergeTreeReaderWide::getStream during a mutation. This is the known Dynamic-column merge bug Logical error "Stream ... variant_discr ... is not found" for a Dynamic column during merge (part written without variant_discr) #107561 (a part written without the variant_discr substream), which has a fix in progress. The accompanying Cannot start clickhouse-server and Check failed entries are downstream consequences within the same stress run.
Stress test (arm_asan_ubsan) → Hung check failed, possible deadlock found is the known flaky Hung check failed, possible deadlock found #107941.

No build was run locally: the merge is additive and the interfaces the combinator overrides are unchanged from the previously-built/tested merge-base, so CI will validate the build and tests.

alexey-milovidov · 2026-06-22T18:32:55Z

Addressed the AI Review's sole remaining (metadata) note: the PR body said Closes #66444, but #66444 is the merged "Implement new JSON data type" PR. Corrected it to Closes: https://github.com/ClickHouse/ClickHouse/issues/59832 (the open "-Sparkbar as an aggregate functions combinator" issue this PR implements), so the right issue auto-closes on merge. AI Review verdict is ✅ Approve, 0 unresolved review threads.

The remaining CI reds are unrelated fuzzer flakes, not caused by this additive combinator:

Stress test (arm_release): Logical error: Unexpected return type from toString. Expected String. Got Nullable(String) in ActionsDAG::evaluatePartialResult — a toString/partial-result fuzzer class (the only sparkbar token in that log is a 'sumSparkbar with Date:' string literal in a fuzzer query whose own error was a benign UNKNOWN_IDENTIFIER).
AST fuzzer (amd_tsan): Block structure mismatch ... different columns in IntersectOrExceptStep::updateOutputHeader — an INTERSECT/EXCEPT query-plan flake.

Master renamed the virtual aggregate-function merge entry point: the public `merge` is now a non-virtual wrapper that asserts the source and destination states do not alias and forwards to a new pure-virtual `mergeImpl`. Overrides must now be done on `mergeImpl`, mirroring how `AggregateFunctionResample` was migrated. Rename the combinator's override accordingly; the body still calls the public `merge` on the nested function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Master grew several tests with the 04342 prefix, so move 04342_sparkbar_combinator_large_range_bucketing to the free 04346 prefix to keep a unique test number (adjacent to 04344/04345 sparkbar tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

alexey-milovidov · 2026-06-28T06:33:29Z

Merged master (the branch was >1 week behind, ~2840 commits). The merge surfaced a real interface change that required a code adaptation:

merge → mergeImpl rename: master made the public IAggregateFunction::merge a non-virtual wrapper that asserts the source and destination states do not alias, and moved the override point to a new pure-virtual mergeImpl. Adapted the -Sparkbar combinator's override accordingly, mirroring how AggregateFunctionResample was migrated (the body still calls the public merge on the nested function). Verified with a -fsyntax-only compile of the combinator translation unit against merged master — clean.
Test renumber: master grew several tests with the 04342 prefix, so moved 04342_sparkbar_combinator_large_range_bucketing to the free 04346 prefix (adjacent to 04344/04345).

Net diff remains purely additive (799 insertions, 0 deletions); the only existing files touched are the 6-line registration in registerAggregateFunctions.cpp and the docs section in combinators.md. No conflicts. 0 unresolved review threads.

clickhouse-gh · 2026-06-29T01:15:35Z

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 153/182 (84.07%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

alexey-milovidov added the can be tested Allows running workflows for external contributors label Mar 31, 2026

clickhouse-gh Bot added the pr-feature Pull request with new product feature label Mar 31, 2026