Repository navigation
Antalya 26.6: add UniqApacheHLL - #2398
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>
3c1bcdb to
8cd0586
Compare
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
|
@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. |
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)
|
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:
|
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>
|
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)
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>
|
@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>
Yup, I just made some fixes and updated regression release branch. I'll rerun aggregate suite to make sure it's working now. |
…r merge them The union inside a state always holds an `HLL_8` sketch; the declared type is applied only when the state is serialized. `size` converted a copy to the declared type just to read its estimate, and `merge` converted the other union to the declared type before feeding it into this one, which accepts a sketch of any type. Read the estimate from the union directly, and merge with its `HLL_8` result. In a standalone micro-benchmark against the vendored DataSketches at `-O2` (not ClickHouse), for `lg_k = 12`: - estimate through a `HLL_4` copy: 72 us; directly on the union: 9 ns - merge through a `HLL_4` result: 88 us; through an `HLL_8` result: 16 us For 60 combinations of `lg_k`, declared type and cardinality, the estimates and the serialized bytes after merging were identical either way. `write` still converts to the declared type, since the serialized state must have it. With the declared type unused, drop that parameter from `size` and `merge`. Tests `04327`, `05137`, `05138` and `05139` pass on a local Debug build. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: UnamedRus <dtitmoav@gmail.com>
The comment claimed that folding the pending sketch last avoids building a union at the declared `lg_k` and downsampling it. That was read from the library source, never measured, and nothing depends on it; the code is correct in either order. Keep the comment about `rhs` holding both sketches, which explains why there are two separate branches. Comment only. Related: #2398 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: UnamedRus <dtitmoav@gmail.com>
|
Hi, ive pushed some changes, ie ignore nullable typing (bc its how apache hll works) but this changed sketches state, ie removed 01 byte prefix if nullable types are used. So we need to regenerate regression suite |
|
https://github.com/Altinity/clickhouse-regression/actions/runs/37389108443 Updated regression suite run, all aggregate functions suites passing. |
CI triage for
|
| Check | Class | PR-caused? |
|---|---|---|
| DCO | sign-off formality | No (process) |
| Grype Scan — keeper (4 High) | base-image CVEs | No |
Grype Scan — server -alpine (1 High) |
base-image CVEs | No |
| GrypeScanServer / GrypeScanKeeper jobs | mirror of the above | No |
Regression release s3_azure_1 |
Azure env / credentials | No |
1. DCO — ACTION_REQUIRED
Several commits on the branch have no Signed-off-by trailer (e.g. 9c9c7fb, 562352f, 81fb83c, 00dbe28 by Julian, the Claude <noreply@anthropic.com> commits a626ce8/516fd39/5cc6aaa, and a number of UnamedRus commits). The earlier 1bfd0e1 "DCO remediation commit" only covered part of them.
This is about commit metadata, not the diff. Fix (needs a human with write access to the branch — I can't rewrite history or force-push): either
git rebase --signoff origin/antalya-26.6then force-push the branch, or- simply squash-merge the PR — a single signed-off merge/squash commit satisfies DCO without rewriting the branch.
2. Grype scans — keeper & server(-alpine)
These scan the built Docker images for OS-package CVEs. The failing ones are all Alpine 3.21 / NVD distro CVEs in the base image:
- keeper (4 High):
CVE-2026-85091,CVE-2026-84782,CVE-2026-84784,CVE-2026-72897 - server-alpine (1 High): same Alpine 3.21 family.
This PR changes only src/AggregateFunctions/*, contrib/datasketches-cpp, and tests/queries/0_stateless/* — it adds no OS packages and touches no Dockerfile, so it cannot introduce these. They are pre-existing on the base branch and affect all PRs built from the same base image. Not PR-related; resolved by a base-image bump, handled outside this PR.
3. Regression release s3_azure_1 — FAILURE (2 scenarios)
Failure is in the Azure disk_invalid suite (s3/tests/disk_invalid.py → access_default). The test deliberately feeds an invalid Azure storage config and asserts the server log contains "Server failed to authenticate". Instead Azure returned a different error:
Code: 347. DB::Exception ... type: Azure::Storage::StorageException,
e.what() = 403 This request is not authorized to perform this operation.
... (CANNOT_LOAD_CONFIG)
assert message in r.output ^ is 'Server failed to authenticate' -> False
This is a mismatch between the expected auth-failure wording and the actual Azure 403 ... not authorized response — an Azure-side/credentials/environment condition in the regression runner, entirely within the Azure object-storage path. The PR touches nothing under src/Disks/**, and the three sibling suites (s3_aws_s3_1/2, s3_azure_2, s3_export_*, both parquet*) are green. Not PR-related — infra/flaky; safe to re-run RegressionTestsRelease / S3 (azure, 1).
Bottom line
No code change is needed on this PR to address CI — all failures are process (DCO) or environment/base-image (Grype, Azure regression). The only actionable item for the PR itself is the DCO sign-off, best handled by squash-merging or a --signoff rebase at merge time. The uniqApacheHLL functionality itself is fully green across builds and functional/integration/aggregate-function regression suites.
— @blau-ai · evidence: praktika result_pr.json, Grype results.html, and the s3_azure_1 regression report.html for 5f06020.
… when #2398 merges Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 participants
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: