[UUID 7/8] UUID partitioning - #18875
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18875 +/- ##
============================================
- Coverage 66.65% 66.61% -0.04%
Complexity 1423 1423
============================================
Files 3443 3447 +4
Lines 218632 218995 +363
Branches 34793 34888 +95
============================================
+ Hits 145726 145884 +158
- Misses 61192 61374 +182
- Partials 11714 11737 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7836d91 to
f9a25e9
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found critical mixed-version compatibility issues; see inline comments.
| // does not contain this enum constant. This is safe because DataSchema serialization uses enum names (not ordinals) | ||
| // via ColumnDataType.name() / ColumnDataType.valueOf(). If ordinal-based serialization is ever added for | ||
| // ColumnDataType, UUID must be moved to the end of the enum (as was done for FieldSpec.DataType.UUID). | ||
| UUID(BYTES, null) { |
There was a problem hiding this comment.
This makes UUID a new broker/server wire-visible ColumnDataType. DataSchema.toBytes() emits the enum name, and older peers still throw in parseColumnDataType(...) once they see UUID, so rolling upgrades and rollback are unsafe as soon as UUID-typed results are in flight. Please keep the wire representation on an existing type until all peers are upgraded, or add an explicit mixed-version compatibility path plus coverage.
| // does not know UUID = 22 / UUID_ARRAY = 23 will fail planning with UnknownEnumValueException when receiving a plan | ||
| // that includes a UUID literal. Avoid issuing UUID queries until all brokers and servers are upgraded. See the | ||
| // matching note on DataSchema.toBytes and ProtoExpressionToRexExpression#convertColumnDataType. | ||
| UUID = 22; |
There was a problem hiding this comment.
This introduces new UUID / UUID_ARRAY proto enum values with no compatibility path for older MSQ peers. Older brokers/servers decode them as UNRECOGNIZED and throw in convertColumnDataType(...), so a UUID literal can break mixed-version planning before execution even starts. Please encode UUID literals using an existing wire type until the cluster is homogeneous, or add version-gated dual-read/dual-write behavior with mixed-version tests.
bc8adc6 to
d74ada7
Compare
ca057a9 to
730a886
Compare
730a886 to
0dde321
Compare
0dde321 to
f2bbaf2
Compare
dbb14c6 to
91a71d8
Compare
Adds the UUID scalar function surface on top of the logical UUID type: - IS_UUID / TO_UUID: polymorphic over STRING and BYTES, sharing dispatch via AbstractStringOrBytesUuidFunction - UUID_TO_BYTES, BYTES_TO_UUID, UUID_TO_STRING, UUID_VERSION, UUID_TIMESTAMP - UUID_V4 / UUID_V7 generators, marked isDeterministic = false so the broker's CompileTimeFunctionsInvoker does not fold one value across every row - Matching multi-stage Udf wrappers so the functions are registered with the MSE UDF registry Depends only on UuidUtils (apache#18869) and ColumnDataType.UUID (apache#18871), both already on master. Split out of apache#18875 so it can be reviewed independently of the UUID partitioning work.
Scalar UDFs split out into #19091The UUID scalar functions and their multi-stage UDF wrappers have been moved out of this PR into #19091, so they can be reviewed and merged in parallel with the partitioning work. Moved to #19091 (15 files):
Still here — #19091 is based directly on Also in this push:
Verified after the split: |
Adds the UUID scalar function surface on top of the logical UUID type: - IS_UUID / TO_UUID: polymorphic over STRING and BYTES, sharing dispatch via AbstractStringOrBytesUuidFunction - UUID_TO_BYTES, BYTES_TO_UUID, UUID_TO_STRING, UUID_VERSION, UUID_TIMESTAMP - UUID_V4 / UUID_V7 generators, marked isDeterministic = false so the broker's CompileTimeFunctionsInvoker does not fold one value across every row - Matching multi-stage Udf wrappers so the functions are registered with the MSE UDF registry Depends only on UuidUtils (apache#18869) and ColumnDataType.UUID (apache#18871), both already on master. Split out of apache#18875 so it can be reviewed independently of the UUID partitioning work.
Adds the UUID scalar function surface on top of the logical UUID type: - IS_UUID / TO_UUID: polymorphic over STRING and BYTES, sharing dispatch via AbstractStringOrBytesUuidFunction - UUID_TO_BYTES, BYTES_TO_UUID, UUID_TO_STRING, UUID_VERSION, UUID_TIMESTAMP - UUID_V4 / UUID_V7 generators, marked isDeterministic = false so the broker's CompileTimeFunctionsInvoker does not fold one value across every row - Matching multi-stage Udf wrappers so the functions are registered with the MSE UDF registry Depends only on UuidUtils (apache#18869) and ColumnDataType.UUID (apache#18871), both already on master. Split out of apache#18875 so it can be reviewed independently of the UUID partitioning work.
* Add UUID scalar functions and multi-stage UDF wrappers Adds the UUID scalar function surface on top of the logical UUID type: - IS_UUID / TO_UUID: polymorphic over STRING and BYTES, sharing dispatch via AbstractStringOrBytesUuidFunction - UUID_TO_BYTES, BYTES_TO_UUID, UUID_TO_STRING, UUID_VERSION, UUID_TIMESTAMP - UUID_V4 / UUID_V7 generators, marked isDeterministic = false so the broker's CompileTimeFunctionsInvoker does not fold one value across every row - Matching multi-stage Udf wrappers so the functions are registered with the MSE UDF registry Depends only on UuidUtils (#18869) and ColumnDataType.UUID (#18871), both already on master. Split out of #18875 so it can be reviewed independently of the UUID partitioning work. * Fix UUID-to-string scalar evaluation * Support polymorphic UUID scalar inputs * Harden polymorphic UUID scalar support
91a71d8 to
b3ad87b
Compare
There was a problem hiding this comment.
Pull request overview
Adds UUID-aware partitioning and related UUID semantics across planning/runtime so UUID values are hashed/serialized consistently as canonical RFC-4122 strings (or raw 16-byte form where required), aligning Pinot’s behavior with common external producers.
Changes:
- Introduces
UuidPartitionFunction(Murmur2 over 16-byte UUID) and threads logicalFieldSpec.DataTypeinto partitioner construction so UUID values render canonically during partitioning. - Improves UUID handling in multi-stage runtime (join/group-by key normalization) and in planner/proto/literal plumbing so UUID literals and UUID_ARRAY are preserved correctly.
- Extends tests across runtime/planner/core to lock in UUID literal rendering, predicate behavior, pruning, and aggregation/group-by correctness.
Reviewed changes
Copilot reviewed 74 out of 74 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtilsTest.java | Tests UUID IN literal emission |
| pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/HashJoinOperatorTest.java | Adds UUID join coverage |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java | UUID literal serialization fix |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/join/UuidLookupTable.java | UUID-optimized lookup table |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/join/LookupTable.java | Adds key normalization hook |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/HashJoinOperator.java | Normalizes join keys for UUID |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/OneUuidKeyGroupIdGenerator.java | UUID group-id generator |
| pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/GroupIdGeneratorFactory.java | Wires UUID generator |
| pinot-query-planner/src/test/java/org/apache/pinot/query/planner/serde/RexExpressionSerDeTest.java | UUID literal serde test tweaks |
| pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverterTest.java | Tests UUID_ARRAY conversion |
| pinot-query-planner/src/test/java/org/apache/pinot/query/parser/CalciteRexExpressionParserTest.java | Tests UUID literal parsing behavior |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/serde/RexExpressionToProtoExpression.java | Notes UUID_ARRAY stored type |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/serde/ProtoExpressionToRexExpression.java | Improves proto type error context |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/v2/PRelToPlanNodeConverter.java | UUID type mapping ordering |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java | UUID Rex literal conversions |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java | UUID type mapping ordering |
| pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java | UUID literal emits CAST(UUID) |
| pinot-core/src/test/java/org/apache/pinot/core/query/pruner/BloomFilterSegmentPrunerTest.java | Adds UUID bloom pruning test |
| pinot-core/src/test/java/org/apache/pinot/core/query/distinct/table/BytesDistinctTableTest.java | Tests UUID distinct formatting |
| pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryGroupKeyGeneratorTest.java | Adds UUID/boolean/timestamp cases |
| pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountHLLAggregationFunctionTest.java | UUID HLL regression tests |
| pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CastTransformFunctionTest.java | UUID cast tests + negatives |
| pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java | UUID CASE tests |
| pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunctionTest.java | UUID compare tests |
| pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunctionTest.java | UUID test column + assertions |
| pinot-core/src/test/java/org/apache/pinot/core/operator/filter/predicate/NoDictionaryInPredicateEvaluatorTest.java | UUID IN predicate tests |
| pinot-core/src/main/java/org/apache/pinot/core/segment/processing/partitioner/TableConfigPartitioner.java | Type-aware partition string rendering |
| pinot-core/src/main/java/org/apache/pinot/core/segment/processing/partitioner/PartitionerFactory.java | Threads schema type into partitioners |
| pinot-core/src/main/java/org/apache/pinot/core/segment/processing/mapper/SegmentMapper.java | Passes schema to partitioners |
| pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java | UUID group-key bytes handling |
| pinot-core/src/main/java/org/apache/pinot/core/query/reduce/filter/PredicateRowMatcher.java | UUID row-matcher conversion |
| pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ValueBasedSegmentPruner.java | Unwrap ByteArray for hashing |
| pinot-core/src/main/java/org/apache/pinot/core/query/distinct/table/BytesDistinctTable.java | Format UUID vs BYTES correctly |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/utils/ValueToIdMapFactory.java | Adds UUID map specialization |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/utils/UuidToIdMap.java | UUID ValueToIdMap impl |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionarySingleColumnGroupKeyGenerator.java | UUID key normalization to UuidKey |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java | UUID on-the-fly dict support |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java | Reject UUID inputs clearly |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java | UUID hashes canonical strings |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java | UUID treated as STRING updates |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountHLLPlusAggregationFunction.java | UUID offers canonical strings |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountHLLAggregationFunction.java | UUID offers canonical strings |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountCPCSketchAggregationFunction.java | UUID updates with canonical strings |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java | UUID hashed into bitmap |
| pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AnyValueAggregationFunction.java | Preserves UUID logical type |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java | UUID_ARRAY MV bytes handling |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java | UUID IN uses UuidKey set |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/IdentifierTransformFunction.java | UUID string rendering from bytes |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CastTransformFunction.java | Adds UUID/UUID_ARRAY casting |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java | Validates UUID literals in CASE |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java | Improves bytes/uuid compare errors |
| pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java | UUID string conversion + guards |
| pinot-core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java | UUID distinct via canonical strings |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/RangePredicateEvaluatorFactory.java | UUID range bounds parsing |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/PredicateUtils.java | UUID stored value for dict lookup |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/NotInPredicateEvaluatorFactory.java | UUID NOT IN evaluator |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/NotEqualsPredicateEvaluatorFactory.java | UUID NEQ evaluator wiring |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/InPredicateEvaluatorFactory.java | UUID IN evaluator wiring |
| pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/EqualsPredicateEvaluatorFactory.java | UUID EQ evaluator wiring |
| pinot-common/src/test/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriterTest.java | UUID predicate rewrite tests |
| pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java | Ensures UUID generators aren’t folded |
| pinot-common/src/test/java/org/apache/pinot/common/request/context/RequestContextUtilsTest.java | UUID RHS-literal folding tests |
| pinot-common/src/test/java/org/apache/pinot/common/request/context/LiteralContextTest.java | UUID literal canonicalization test |
| pinot-common/src/test/java/org/apache/pinot/common/partition/function/PartitionFunctionTest.java | Adds UuidPartitionFunction test |
| pinot-common/src/test/java/org/apache/pinot/common/function/FunctionUtilsTest.java | UUID type mapping tests |
| pinot-common/src/main/proto/expressions.proto | Adds UUID/UUID_ARRAY enum values |
| pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java | Literal detection supports foldables |
| pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/BaseInPredicate.java | Caches parsed UUID literals |
| pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java | Normalizes UUID values to java.util.UUID |
| pinot-common/src/main/java/org/apache/pinot/common/partition/function/UuidPartitionFunction.java | New UUID partition function |
| pinot-common/src/main/java/org/apache/pinot/common/function/scalar/array/ArraysOverlapScalarFunction.java | Adds UUID_ARRAY support |
| pinot-common/src/main/java/org/apache/pinot/common/function/scalar/array/ArrayLengthScalarFunction.java | Adds UUID_ARRAY support |
| pinot-common/src/main/java/org/apache/pinot/common/function/FunctionUtils.java | Adds UUID mapping helpers |
| Object normalizedKey = key != null ? UuidKey.fromObject(key) : null; | ||
| if (_groupIdMap.size() < _numGroupsLimit) { | ||
| return _groupIdMap.computeIfAbsent(normalizedKey, _groupIdGenerator); | ||
| } else { | ||
| return _groupIdMap.getInt(normalizedKey); | ||
| } |
| private String toPartitionString(Object value) { | ||
| return _dataType != null ? _dataType.toString(value) : FieldSpec.getStringValue(value); | ||
| } |
3cead21 to
64aeae4
Compare
54b9741 to
9e87ef7
Compare
Part 5/8 of splitting apache#18140 (logical UUID type). Rebased onto latest master; stacked on uuid-split/04-sse-predicates-cast. Downstream references use the UuidKey class merged in apache#18869.
… path getConvertedKey had `case UUID` falling through to BYTES, returning the raw byte[]. The other reduce path converts group keys via ColumnDataType#convert, and UUID is the one type whose converted form is not its stored bytes -- it yields a java.util.UUID. PredicateRowMatcher casts that directly (see apache#18872), so the byte[] made GROUP BY ... HAVING over a UUID column fail with "ClassCastException: class [B cannot be cast to class java.util.UUID". Delegating to columnDataType.convert(...) keeps the two paths identical by construction rather than by duplicated knowledge. Covered by a new UuidAggregationTest integration test rather than a unit test. The unit-level BaseQueriesTest harness cannot reach this code, which is why it was uncovered; a query-level test goes through the real broker reduce. Verified by reverting the fix: testGroupByUuidColumnWithHaving and testGroupByUuidColumnWithHavingReturningFinalResult both fail with the ClassCastException and both pass with it. The test also covers GROUP BY key rendering, DISTINCT (BytesDistinctTable no longer hard-codes hex) and DISTINCTCOUNT / DISTINCTCOUNTHLL / DISTINCTCOUNTBITMAP over a UUID column.
Both callers key on UuidKey already (NoDictionary{Single,Multi}ColumnGroupKey
Generator, via UuidKey.fromBytes), so UuidKey.fromObject accepted five input
types where exactly one is ever passed, and ran an instanceof chain per row in
the group-by loop.
Casting directly matches the sibling maps -- DoubleToIdMap casts to double --
and keeps the input type deterministic, which is what was asked for on the
equivalent PredicateRowMatcher branch in apache#18872.
…cal string The previous version rendered each UUID as its 36-char canonical string so that DISTINCTCOUNTHLL(uuidCol) would equal DISTINCTCOUNTHLL(CAST(uuidCol AS STRING)). No other logical type provides that guarantee: the scan path switches on the stored type, so TIMESTAMP offers its raw millis and BOOLEAN its int, and neither matches a CAST to STRING. The UUID rendering was inventing a cross-type equivalence at the cost of a String allocation per row in the aggregation loop. UUID now hashes its stored 16 bytes. Verified byte[] is content-hashed rather than identity-hashed by both HyperLogLog (clearspring MurmurHash) and UltraLogLogUtils.OBJECT_FUNNEL (putBytes). A minimal guard is still needed at each site, because unlike LONG or INT the stored BYTES type is not a scalar case in this family: the scan path has no `case BYTES`, and the dictionary path reads BYTES as serialized sketch state. - AggregationFunctionUtils: the three UUID blocks are gone; the BYTES guard now excludes UUID so it falls through to the scalar path, which offers dictionary.get(i) -- the stored byte[] -- exactly as the scan path does. - DistinctCountBitmap hashes Arrays.hashCode(bytes). - DistinctCountThetaSketch cannot take scalar bytes (BYTES there means "serialized sketch"), so it surfaces the stored hex rendering instead. Replaces testUuidDistinctCountHllMatchesStringDistinctCountHll, which asserted the invariant being dropped, with one pinning the new behaviour.
Part 6/8 of splitting apache#18140 (logical UUID type). Rebased onto latest master; stacked on uuid-split/05-agg-groupby-distinct. Downstream references use the UuidKey class merged in apache#18869.
Part 7/8 of splitting apache#18140 (logical UUID type). - UuidPartitionFunction: hashes the 16-byte UUID form via Murmur2, matching what an external producer keyed on raw UUID bytes computes - PartitionerFactory / TableConfigPartitioner: thread the column's logical DataType through so UUID columns render canonically instead of as bare hex - UUID_ARRAY entries for the array scalar functions The UUID scalar functions and multi-stage UDF wrappers that were previously part of this layer now live in their own PR (apache#19091) so they can be reviewed and merged in parallel.
9e87ef7 to
903b940
Compare
Parent tracking issue: #16619
What
UUID partitioning.
Changes
UuidPartitionFunction— parses the canonical UUID string into its 16-byte form, hashes those bytes via Murmur2, then applies the configuredPartitionIdNormalizer. This matches what an external producer keyed on raw UUID bytes computes, which is the common convention for UUID-keyed messages in Kafka/Pulsar/Kinesis.PartitionerFactory/TableConfigPartitioner— thread the column's logicalFieldSpec.DataTypethrough so values render viaDataType.toString(Object)(canonical UUID strings) instead of the bare hexFieldSpec.getStringValuewould emit. Behavior is unchanged for every non-UUID type.UUID_ARRAYentries for the array scalar functions (arrayLength,arraysOverlap).Scope change
The UUID scalar functions and multi-stage UDF wrappers that used to be part of this layer have moved to #19091, which is based directly on
masterand can be reviewed and merged in parallel with this PR.About this PR / how to review
This is part 7 of 8 splitting #18140 (first-class logical
UUIDtype) into layered PRs, as requested there.The split is enabled by the v1 design:
DataType.UUIDhas stored typeBYTES, so most paths handle it automatically; each PR adds explicit UUID semantics to one subsystem. Head branch lives onxiangfu0/pinot.This PR is stacked on #18874 (branch
uuid-split/06-mse-planner-runtime). Because GitHub PRs to apache must base onmaster, the Files-changed tab is cumulative (it includes layers 4–7) until the PRs below it merge. Review the commit titled[UUID 7/8] UUID partitioning— that is this layer's change. Each parent merge shrinks this diff after a rebase.Full stack (merge bottom → top)
[UUID 1/8] Add logical UUID type foundation (pinot-spi) #18869 — [UUID 1/8] logical UUID type foundation (pinot-spi)merged[UUID 2/8] UUID ingest and segment storage #18870 — [UUID 2/8] UUID ingest and segment storagemerged[UUID 3/8] UUID result rendering (DataSchema, Arrow/JSON encoders) #18871 — [UUID 3/8] UUID result rendering (DataSchema, Arrow/JSON encoders)mergedIndependent of the stack: #19091 — UUID scalar functions and multi-stage UDF wrappers (based on
master, reviewable now).Full feature description, v1 design contract, scope exclusions, and benchmark numbers: #18140.