[UUID 8/8] UUID integration tests, benchmarks and docs#18876
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18876 +/- ##
============================================
- Coverage 64.80% 64.79% -0.02%
Complexity 1347 1347
============================================
Files 3392 3413 +21
Lines 211769 213632 +1863
Branches 33345 33707 +362
============================================
+ Hits 137232 138414 +1182
- Misses 63451 63994 +543
- Partials 11086 11224 +138
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:
|
93272e3 to
4856ac5
Compare
xiangfu0
left a comment
There was a problem hiding this comment.
Found critical mixed-version compatibility issues inherited in this stack head; 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.
7b54127 to
1eb0c13
Compare
Part 2/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on 5a451fa. Downstream references use the UuidKey class merged in apache#18869.
Part 3/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on uuid-split/02-ingest-storage. Downstream references use the UuidKey class merged in apache#18869.
Part 4/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on uuid-split/03-result-rendering. Downstream references use the UuidKey class merged in apache#18869.
Part 5/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on uuid-split/04-sse-predicates-cast. Downstream references use the UuidKey class merged in apache#18869.
Part 6/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); 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). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on uuid-split/06-mse-planner-runtime. Downstream references use the UuidKey class merged in apache#18869.
Part 8/8 of splitting apache#18140 (logical UUID type). Rebased onto master (now includes the merged apache#18869 type foundation); stacked on uuid-split/07-udfs-partitioning. Downstream references use the UuidKey class merged in apache#18869.
1eb0c13 to
ce33dae
Compare
What
End-to-end verification and documentation.
Changes
UuidTypeTest,UuidTypeRealtimeTest,UuidUpsertRealtimeTest(extendCustomDataQueryClusterIntegrationTest)BenchmarkUuidGroupingAndLookup,BenchmarkUuidQueryExecutionREADMEusage / limitations / migrationEnables
Offline + realtime + upsert integration coverage and performance benchmarks. The top of the stack — its cumulative diff equals the original #18140.
Depends on
#18869, #18871 and #18874 (benchmarks import multi-stage runtime classes).
About this PR / how to review
This is part 8 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 #18875 (branch
uuid-split/07-udfs-partitioning). Because GitHub PRs to apache must base onmaster, the Files-changed tab is cumulative (it includes layers 1–8) until the PRs below it merge. Review the commit titled[UUID 8/8] UUID integration tests, benchmarks and docs— that is this layer's change. Each parent merge shrinks this diff after a rebase.Full stack (merge bottom → top)
After #18869 lands, PRs 2/4/7 (#18870, #18872, #18875) only depend on it and can be reviewed in parallel; PRs 5/6 (#18873, #18874) also need #18871; PR 8 (#18876) needs #18874.
Full feature description, v1 design contract, scope exclusions, and benchmark numbers: #18140.