Conversation
Backport of ClickHouse/ClickHouse@a067567, re-created here rather than cherry-picked so that it carries a sign-off. The submodule tracked `apache/datasketches-cpp` directly, pinned to `76edd74f` (2024-05-16), an upstream development commit. It moves onto a `ClickHouse/`-prefixed branch of our fork, the way `docs/development/contrib` asks: ClickHouse/datasketches-cpp, branch ClickHouse/5.2.0 de8553ba 5.2.0, the newest upstream release (2025-01-15) 23bd9b07 backport of apache/datasketches-cpp#512 apache/datasketches-cpp#512 is the HyperLogLog union fix. No upstream release carries it, so it is cherry-picked onto the `5.2.0` tag there. `uniqApacheHLL`, added next, needs it: without it a merged HLL sketch reports a wrong estimate and serializes a state that other DataSketches implementations read differently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: UnamedRus <dtitmoav@gmail.com>
…tion
Ported from `vk/uniq-apache-hll` (github.com/UnamedRus/ClickHouse), which develops the
function against `master`.
`uniqApacheHLL` counts distinct values into an Apache DataSketches HLL sketch, and its
`-State` serializes that sketch in the DataSketches format, so states can be exchanged
with Java, Python and C++ services through the standard `-State`/`-Merge` combinators.
Only the argument types those libraries hash the same way are accepted - integers of at
most 64 bits, Enum8/16, BFloat16, Float32/64, String, FixedString, UUID, IPv4, IPv6,
Date, Date32, DateTime and DateTime64 - so no state can be built here that an external
consumer cannot reproduce.
Two deviations from the branch this is taken from, both forced by the age of this base:
- the state overrides `merge`, not `mergeImpl`. `IAggregateFunction::merge` is still
the pure virtual here; the split into a non-virtual `merge` plus a `mergeImpl`
override came later.
- `introduced_in` says 26.6 rather than 26.9, this being the release it ships in.
NOT BUILT OR TESTED on this base - only ported and checked by inspection against the
26.6 headers. CI is the first real build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
UnamedRus
force-pushed
the
feature/antalya-26.6/uniq-apache-hll
branch
from
September 17, 2026 12:16
3c1bcdb to
8cd0586
Compare
9 of 30 tasks
This comment was marked as outdated.
This comment was marked as outdated.
zvonand
changed the title
Reuse `assertUnary`, consolidate state handling, and shorten repetitive comments and documentation. Merge overlapping HLL tests while retaining interoperability, cross-resolution, corruption, and dependency coverage. Use the canonical `Enum` documentation type to avoid a logical-error exception when reading `system.functions`. Validation: rebuilt ClickHouse and passed all four remaining PR tests with the pinned DataSketches revision; no tests skipped. Related: #2398
Member
Member
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Collaborator
CI triage — @zvonandVerdict: Only one class of failure is caused by this PR, and it's the expected one — the regression suite noticing the new function isn't covered yet. Everything else is pre-existing / flaky / infra. Nothing points to a bug in the Head SHA analyzed: 🔴 PR-caused (1 root cause — action needed, but not in this repo)
This is the Altinity clickhouse-regression Fix: add 🟡 Not PR-caused — pre-existing / flaky / infra (safe to re-run or ignore)Stateless tests — all failures are unrelated to aggregate functions and are confirmed transient by CI's own auto-rerun:
Stress test (amd_asan_ubsan, cas s3 storage) — Integration tests (arm_binary, distributed plan, 3/4) — Grype Scan (keeper + server-alpine) — Other regression suites — TL;DR
🤖 Automated CI triage by @blau-ai. Evidence: praktika |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c9c7fbb49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ation - Construct `hll_union` with the requested `lg_k` instead of flooring it at 7, so `uniqApacheHLLMerge(4..6)` produces sketches with the requested `lg_k`. `hll_union` accepts `lg_max_k` in [4, 21]. - Serialize an untouched state as the library's compact empty sketch instead of a zero-length vector, so external DataSketches consumers can read it. Related: #2398 (comment) Related: #2398 (comment)
Collaborator
|
AI audit note: This review comment was generated by AI. Audit update for PR #2398 ( Confirmed defects: Medium: Fast test and any
Medium: Empty
Coverage summary:
|
CarlosFelipeOR
added a commit
to Altinity/clickhouse-regression
that referenced
this pull request
Oct 1, 2026
uniqApacheHLL is an Antalya-only aggregate function (Altinity/ClickHouse#2398) whose state is compatible with Apache DataSketches HLL. The suite reuses the any checks for supported types and adds checks for rejected types, HLL estimation mode, lg_k and HLL type parameters, empty strings, and reading an external DataSketches state. Skipped on non-Antalya builds and before 26.6. Snapshots are x86_64 only; aarch64 follows. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: CarlosFelipeOR <carlosfelipeor@gmail.com>
Collaborator
|
I added SELECT uniq(NULL), uniqTheta(NULL); -- 0 0
SELECT uniqApacheHLL(NULL); -- Code: 43. Illegal type Nullable(Nothing) of argument for aggregate function uniqApacheHLLThe failing checks are AI analysis: for an all-NULL argument, the factory still calls the creator with |
For `Nullable(Nothing)` the factory creates the nested function before the `Null` combinator replaces it with `nothing`, so `uniqApacheHLL(NULL)` threw `ILLEGAL_TYPE_OF_ARGUMENT` instead of returning 0 like `uniq` and `uniqTheta`. Merge `05026_uniq_apache_hll_interop` into `04327_uniq_apache_hll` and add the all-NULL cases there. `05136_uniq_apache_hll_corrupted_state.sh` stays a shell test because `CORRUPTED_DATA` is always logged with a stack trace to stderr. #2398 (comment)
11 of 30 tasks
Once a union existed, every inserted row created a sketch, merged it into the union and freed it. Keep inserting into the update sketch and fold it into the union only when the state is merged, read or serialized. Merging a state that holds both sketches now takes both without modifying it. Include `<bit>`, `<new>` and `<exception>` for `std::byteswap`, `std::bad_alloc` and `std::exception`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jim6F2cKTTpxvY82Kq8a28
… states A fractional or negative `lg_k` was silently truncated or wrapped; it now fails with `BAD_ARGUMENTS`. Merging an empty state no longer allocates a union. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jim6F2cKTTpxvY82Kq8a28
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jim6F2cKTTpxvY82Kq8a28
…lg_k` Return to the conversion the other `uniq*` functions use. Non-finite, out of range and non-numeric values already fail with `CANNOT_CONVERT_TYPE`, and a negative integer wraps and is rejected by the `[4, 21]` range check with `ARGUMENT_OUT_OF_BOUND`. Only an in-range fraction such as `12.5` is truncated. Drop the `12.5` test and expect `ARGUMENT_OUT_OF_BOUND` for `-1`. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…gistration `registerAggregateFunctions.cpp` only got `USE_DATASKETCHES` transitively through `IColumn.h`. If that include chain changed, `#if USE_DATASKETCHES` would become false without any build error and `uniqApacheHLL` and `uniqTheta` would stop being registered. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…le bounds check Moves the submodule from `23bd9b07` to `4e917778`, the tip of `ClickHouse/datasketches-cpp` branch `ClickHouse/5.2.0`. The new commit checks the buffer size before `compact_theta_sketch_parser::parse` reads the `num_entries` and `theta` fields, which an AST fuzzer found to read out of bounds on truncated `uniqTheta` states: ClickHouse#119595 It does not touch the HLL code used by `uniqApacheHLL`. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Member
|
@DimensionWieldr aggregate suite is still failing. could you pls take a look? |
In `merge`, take `rhs.sk_update` and `rhs.sk_union` first and fold this state's own pending `sk_update` into the union last. When `rhs` has a lower resolution than the declared `lg_k`, the union is then created at that resolution directly, instead of being built at `lg_k` and downsampled by `hll_union::update`. The merged result is the same either way. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…border values `04327_uniq_apache_hll` pins byte-exact states only for `UInt64`, `UUID`, `IPv6` and `DateTime64(3)`. Everything else is checked by cardinality alone, so a change in how `Float64`, `IPv4`, `Date` or `String` values are hashed would break interoperability with other DataSketches implementations without failing any test. The new test feeds each supported type its border values, such as the minimum and maximum of every integer width, values at and above the sign bit of the unsigned types, `-0`, `NaN`, infinities and denormals, `FixedString` padding, negative `Date32` and `DateTime64`, and compares the serialized state with the bytes written by the Apache DataSketches C++ library. It also pins the empty sketch for several `lg_k` and storage types, the list, set and dense modes, and that types share the hash of their underlying value. Integers are widened by value to 64 bits, as Java `update(long)` and Python `update(int)` do. The C++ overloads for narrow unsigned types sign-extend instead, so a `UInt32` of `4294967295` hashes differently from `Int32` `-1`; the test pins that. The expected output was generated by the C++ library and cross-checked against two fixtures already in `04327_uniq_apache_hll`. It has not been run against a server. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Apache DataSketches implementations skip empty strings: Java `update(String)` and `update(byte[])`, the C++ `update(std::string)` (and so Python `update(str)`), and Spark's `hll_sketch_agg`, which also ignores `NULL`. `uniqApacheHLL` counted `''` as a value, so a sketch of data that contains empty strings differed from one built outside ClickHouse by one value. Skip them in `add`, which also makes the state of empty strings only the empty sketch. `NULL` values were already ignored by the `Null` combinator. Because the function has `returns_default_when_only_null`, that combinator writes its flag byte for every state, even one built from `NULL` values only, so the state of a `Nullable` argument is `0x01` followed by the ordinary sketch. Document it and pin it, together with import of an external sketch into a `Nullable` state type and merging of `Nullable` states, in a new test. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…etches sketch `uniqApacheHLL` has `returns_default_when_only_null`, so the generic `Null` combinator wrote its flag byte (always `1`) before the sketch of every `Nullable` argument. The state of `Nullable` columns, the common case, was then not a DataSketches sketch, and an external consumer would have had to skip the byte, or an importer to add it. Give the function its own null adapter, `AggregateFunctionNullUnary<false, false>`, as `sumCount` and `intervalLengthSum` do. `NULL` rows are still skipped, and the state is the same bytes as for a non-`Nullable` argument, the empty sketch when there were no values. The `If` combinator builds its own null adapter without asking the nested function, so `uniqApacheHLLStateIf` over a `Nullable` argument still writes the flag byte, as the other functions with this property do. Change the test to expect bare sketches, to compare `Nullable` and plain states, and to import an external sketch into a `Nullable` state type without a prefix byte. Not built or run. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…he plain state The previous commit made the bytes of that state a bare DataSketches sketch, but its type still named the `Nullable` argument, so `AggregateFunction(uniqApacheHLL, Nullable(T))` and `AggregateFunction(uniqApacheHLL, T)` could not be mixed: no `UNION ALL`, no `INSERT ... SELECT` into the other column type, and `haveSameStateRepresentation` was false. Replace the generic `AggregateFunctionNullUnary<false, false>` with a `Nullable` variant of the function itself, selected through `getOwnNullAdapter`. It skips `NULL` rows through the null map, overrides `getStateType` and `getNormalizedStateType` to return those of the plain function, whose state layout is identical, and ignores nullability of the arguments in `haveSameStateRepresentationImpl`, so a column declared with a `Nullable` argument stays interchangeable. `LowCardinality` needs no handling: the factory and the aggregator strip it before the function sees types or columns. The generic adapter is left alone; it is shared by `sumCount` and `intervalLengthSum`, whose state types would change. New test `05139` pins the state type, mixing with `UNION ALL`, `CAST` and `INSERT` in both directions, and `LowCardinality(Nullable(String))` arguments. Not built or run. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…appers The wrapper that the `Null` combinator puts around a function for `Nullable` arguments writes a flag byte before the state and gives it a type that names the `Nullable` argument. For `uniqApacheHLL` neither is needed, since the state of no values is the empty sketch, and both stop a state built from a `Nullable` column from being read by another DataSketches implementation or mixed with the state of a plain column. Add `IAggregateFunction::stateIsIndependentOfNullability`, `false` by default, next to `getOwnNullAdapter` and `getArgumentsThatCanBeOnlyNull`. When it returns `true` the `Null` combinator and the `If` combinator over `Nullable` arguments use the existing no-flag wrappers, `AggregateFunctionNullUnary<false, false>` and the variadic and `If` equivalents, and such a wrapper reports the state type of the nested function. `AggregateFunctionIf` forwards the question to its nested function. The wrappers ask the nested function when the type is requested, so nothing is added to their constructors, and every function that does not override the method behaves as before. This replaces the `Nullable` variant of `uniqApacheHLL` from the previous commit, which covered only the `Null` combinator, with the generic mechanism, which also covers `uniqApacheHLLStateIf` and `uniqApacheHLLMergeIf` over `Nullable` arguments. `uniqApacheHLL` keeps `haveSameStateRepresentationImpl` ignoring nullability, so a column declared with a `Nullable` argument stays interchangeable. The test now also covers the state type and bytes of `StateIf` and `MergeIf` with a `Nullable` argument and a `Nullable` condition. Not built or run. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…inity/ClickHouse into review-pr2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rst CI run The first CI run built and ran the tests for the first time. Only these two failed. `05139_uniq_apache_hll_nullable_state_type`: `uniqApacheHLLStateIf` has the type `AggregateFunction(uniqApacheHLL, UInt64)`, not `AggregateFunction(uniqApacheHLLIf, UInt64, UInt8)` as the reference guessed. It is the same for a `Nullable` argument, a plain one and a `Nullable` condition, which is what the test is meant to show. `05137_uniq_apache_hll_hash_borders`: the `Float64` and `Float32` states took the largest values and the denormals as text, and the server parses text with its fast path, which is not correctly rounded: `5e-324` was lost and `3.4028235e38` was not the largest `Float32`. Build the values from their exact bit patterns instead, with `reinterpretAsFloat64` and `reinterpretAsFloat32`. The expected states are unchanged, since they were computed from those bit patterns. CI report: https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2398&sha=e99077e33925c82fab2ed4da8ae16ad2519b9882&name_0=PR Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 71ed5a7 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 2033e7b I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: a30cd57 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 30ad260 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 8d49460 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 31e9e7a I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 254c630 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: b403cd0 I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 12c65dd I, UnamedRus <dtitmoav@gmail.com>, hereby add my Signed-off-by to this commit: 2440dbe Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: UnamedRus <dtitmoav@gmail.com>
Collaborator
Yup, I just made some fixes and updated regression release branch. I'll rerun aggregate suite to make sure it's working now. |
This branch has not been deployed
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
7 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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Aggregate function, which states are compatible with apache data sketches HLL implementation
CI/CD Options
Exclude tests:
Regression jobs to run: