Skip to content

UniqApacheHll fixes based on feedback - #2544

Merged
zvonand merged 9 commits into
antalya-26.6from
feature/antalya-26.6/uniq-apache-hll-extended
Oct 23, 2026
Merged

UniqApacheHll fixes based on feedback#2544
zvonand merged 9 commits into
antalya-26.6from
feature/antalya-26.6/uniq-apache-hll-extended

Conversation

@UnamedRus

@UnamedRus UnamedRus commented Oct 12, 2026 •
edited by xieandrew
Loading

Copy link
Copy Markdown
Collaborator

Changelog category (leave one):

  • Improvement

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

Add new functions addVarUIntLengthPrefix, removeVarUIntLengthPrefix, and toVarUInt to add or strip the length prefix so raw Apache DataSketches can be read as uniqApacheHLL states and exported back. New optional third parameter estimator for uniqApacheHLL with values 'DEFAULT' or 'COMPOSITE'. 'COMPOSITE' gives a result not dependent on the order of inserts and merges.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

UnamedRus and others added 2 commits October 12, 2026 20:14
@UnamedRus @claude
Port the extended uniqApacheHLL from the upstream pull request
3658d94 
Brings the implementation of ClickHouse#124667 to this branch:
- the third parameter `estimator` (`'DEFAULT'` or `'COMPOSITE'`), the second computes the estimate from the registers of the sketch only, so it does not depend on the order of the inserts and of the merges;
- a state that is read keeps the sketch as it was written, and a mixed state is merged in the order in which it is folded;
- states of any parameters have one normalized state type;
- the bytes of a `UUID` do not depend on the byte order of the host;
- the function is marked as order dependent and is not registered on big-endian platforms;
- the documentation tells how to hash `UInt8`, `UInt16` and `UInt32` like the C++ library.

Related: ClickHouse#124667

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
@UnamedRus @claude
Add addVarUIntLengthPrefix, removeVarUIntLengthPrefix and `toVarU…
ab14f31 
…Int`

A sketch that another system writes, for example Spark into Parquet, has no length in front of it, while the state of `uniqApacheHLL` stores its sketch as a `VarUInt` length and the bytes. `CAST` of such a sketch to `AggregateFunction(uniqApacheHLL, ...)` fails with `CORRUPTED_DATA`, and `toString` of a state gives bytes that the other system cannot read.

`addVarUIntLengthPrefix` puts the length of a string in front of it, `removeVarUIntLengthPrefix` takes it off and throws an exception when the length is not complete or is not the number of the bytes that follow, and `toVarUInt` encodes one unsigned number. The length is read as the `RowBinary` reader reads it.

Related: ClickHouse#124667

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
@github-actions

github-actions Bot commented Oct 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Workflow [PR], commit [b075fc1]

UnamedRus and others added 3 commits October 12, 2026 20:40
@UnamedRus @claude
Fold the pending sketch of a uniqApacheHLL state before merging int…
4e0c476 
…o it

A state can hold a union and a sketch of inserts that came after a merge, for example when the rows of the tail of a block are added to a state that has already merged the lookup tables of the block, as the aggregation by a `UInt8` key does. `merge` took the new sketches first and folded the pending sketch at its end, so the older rows came after the newer ones. As the HIP estimate and the bytes of the sketch depend on this order, the pending rows are now folded into the union before the sketches are taken.

The result of the default estimator can still differ with the size of the blocks, since the lookup tables merge the rows in another order; the composite estimator does not, and a test checks it.

Related: ClickHouse#124667

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
@UnamedRus @claude
Shorten the comment of stateIsIndependentOfNullability
3167a8f 
Related: ClickHouse#124667

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
@UnamedRus @claude
Do not use VAR_UINT_MAX_SIZE in removeVarUIntLengthPrefix
521419f 
The constant exists in the `VarInt.h` of upstream master only, so the function did not compile on this branch. It is defined in the file under another name, so that it does not clash when the branch merges upstream.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: UnamedRus <dtitmoav@gmail.com>
@svb-alt
svb-alt requested a review from xieandrew October 20, 2026 14:17
@xieandrew xieandrew mentioned this pull request Oct 21, 2026
Merged
DimensionWieldr
DimensionWieldr reviewed Oct 22, 2026
View reviewed changes
Comment thread src/Functions/varUIntLengthPrefix.cpp

for (size_t row = 0; row < input_rows_count; ++row)
{
const std::string_view value = column.getDataAt(row);

@DimensionWieldr DimensionWieldr Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI audit uncovered:

removeVarUIntLengthPrefix throws on a Nullable(String) column that contains any NULL.

A NULL is stored as an empty nested string. The result type is Nullable(String), and with short_circuit_function_evaluation_for_nulls_threshold = 1 the default null wrapper still runs executeImpl on those rows, so getDataAt is '' and this throws INCORRECT_DATA. One null fails the whole batch.

SELECT removeVarUIntLengthPrefix(if(number = 0, unhex('03616263'), CAST(NULL, 'Nullable(String)'))) FROM numbers(2)

Skip null rows and decode only the rest. The constant-NULL test does not hit this path.

@xieandrew xieandrew Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, the function now skips NULL rows, decodes the rest, and wraps result in Nullable

@xieandrew
Fix removeVarUIntLengthPrefix NULL handling
b075fc1 
Signed-off-by: Andrew Xie <dev@xie.is>
@DimensionWieldr

DimensionWieldr commented Oct 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

CI triage (short summary)

No blockers. Nothing that failed was caused by this pull request.

Head b075fc13, run 39827710383. Two jobs failed:

  • 04023_rollup_totals_memory_limit in Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) expected a memory-limit error and got a CAS read timeout (CAS write could not be committed after 35 attempts). Infrastructure. The other shard of that suite passed.
  • Stress test (amd_debug, cas s3 storage) hung on a cancelled async insert from 03365_json_rollback_bug.sh, still in the process list after 839 seconds. That query is not uniqApacheHLL. Plain debug stress and asan CAS stress both passed. One run, so this is not fully classified, and it does not point at this change.

The fast test and the amd debug stateless suites passed, which is where the new uniqApacheHLL queries run. Aggregate-functions regression was skipped on this run.

@DimensionWieldr

DimensionWieldr commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

CI looks fine.

AI audit returned no defects.

#2545 which is this PR but for 26.8, has CI run aggregate functions 1/2/3 which are passing for the uniqApacheHLL tests (unrelated aarch fails). Not sure why it wasn't checked in the description to run here, but it should be fine.

LGTM

@DimensionWieldr DimensionWieldr added the verified Approved for release label Oct 23, 2026
@zvonand
zvonand merged commit 7e88f8f into antalya-26.6 Oct 23, 2026
311 of 316 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DimensionWieldr DimensionWieldr DimensionWieldr left review comments
@xieandrew xieandrew Awaiting requested review from xieandrew

Assignees

No one assigned

Labels

antalya antalya-26.6 verified Approved for release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@UnamedRus @DimensionWieldr @xieandrew @zvonand @svb-alt