Repository navigation
Conversation
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>
…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>
…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>
Related: ClickHouse#124667 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: UnamedRus <dtitmoav@gmail.com>
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>
PR #2545 CI TriageHead Compared with the previous head The CI database was not queried. The rate comparison is this pull request’s two heads. Summary
What this pull request actually ranx86 Pre-existing (same on
|
|
|
||
| AggregateFunctionProperties properties; | ||
| auto function = AggregateFunctionFactory::instance().get("uniqApacheHLL", NullsAction::EMPTY, normalized_argument_types, {}, properties); | ||
| return std::make_shared<DataTypeAggregateFunction>(function, normalized_argument_types, Array{}); |
There was a problem hiding this comment.
From AI audit:
getNormalizedStateType makes every lg_k / storage type / estimator spelling compare equal, and that is not safe for this state.
array, if, and a squashed UNION ALL only CAST when the types differ. Once these types compare equal, that cast is skipped and concatenation goes through ColumnAggregateFunction::insertFrom / ensureOwnership, which merges each sketch into an empty state of whichever function survives (the first common type). HllSketchData::merge builds that union at the surviving lg_k, and write then persists get_result(target_type). A sketch built at lg_k = 14 placed next to one built at lg_k = 4 is permanently downsampled; hex(toString(...)) no longer round-trips, and a later estimate is the coarser one.
CAST itself would have kept the sketch, because read no longer folds it into a union. The tests only check toTypeName and small cardinalities, which stay exact after the resolution drop.
This is new relative to #2398. That PR already ignores parameters in haveSameStateRepresentationImpl, so an explicit uniqApacheHLLMerge(8) of a higher-resolution state downsamples on purpose. It does not override getNormalizedStateType, so uniqApacheHLL(8) and uniqApacheHLL(14, 'HLL_6') stay different types and this concatenation path does not run.
The estimator is the parameter that does not change the sketch. lg_k and the storage type are applied by merge and write, so they should stay part of the state type.
There was a problem hiding this comment.
Addressed this on #2544 (the 26.6 version of this PR) since that one should be prioritized. Now getNormalizedStateType only compares equal for any estimator and not any lg_k and storage type. Please put further review comments on that PR.
|
@UnamedRus pls either add a changelog entry OR change category to "not for changelog" |
Signed-off-by: Andrew Xie <dev@xie.is>
Signed-off-by: Andrew Xie <dev@xie.is>
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Add new functions
addVarUIntLengthPrefix,removeVarUIntLengthPrefix, andtoVarUIntto add or strip the length prefix so raw Apache DataSketches can be read asuniqApacheHLLstates and exported back. New optional third parameterestimatorforuniqApacheHLLwith values 'DEFAULT' or 'COMPOSITE'. 'COMPOSITE' gives a result not dependent on the order of inserts and merges.