Rework UUID type conversion to use java.util.UUID as the external form

[Jackie-Jiang]

Summary

Establishes a consistent internal/external representation for the UUID type across pinot-spi.

PinotDataType

• java.util.UUID is now the external form and byte[] the internal form. UUID.convert / UUID_ARRAY.convert return UUID / UUID[], while toInternal returns byte[] / byte[][].
• UUID.convert delegates to sourceType.toUUID(...), and every single-value type now defines toUUID. Unsupported conversions (e.g. INT -> UUID) throw a descriptive Cannot convert value from INT to UUID rather than the confusing There is no single-value type ... that leaked from the generic fallback. This mirrors how each type already overrides toBytes / toTimestamp.
• JSON→UUID parsing moves into JSON.toUUID. The now-unused toUuidBytesArray is removed.

FieldSpec.DataType

• UUID values are represented uniformly as byte[], matching the BYTES stored type. equals / hashCode / compare / toString drop the toBytesValue indirection and operate on byte[] directly.
• Adds enum-level Javadoc documenting the in-memory Java class each type's values are held as, and calls out that convertInternal returns the internal storage form (ByteArray for BYTES / UUID).

UuidUtils

• toBytes(String) accepts the dashless 32-char hex form in addition to the canonical string, so byte[] default null values (stringified as raw hex) round-trip back through FieldSpec.getDefaultNullValue.

[Copilot]

Pull request overview

This PR standardizes Pinot’s UUID handling by making java.util.UUID the external representation in pinot-spi (PinotDataType), while keeping UUID stored internally as fixed 16-byte byte[] (and ByteArray where Pinot uses internal wrappers). It also extends UUID parsing to accept dashless 32-char hex strings so UUID default-null values can round-trip through string forms.

Changes:

• Reworks PinotDataType UUID/UUID_ARRAY conversion APIs so convert(...) returns UUID / UUID[], while toInternal(...) returns byte[] / byte[][].
• Updates FieldSpec.DataType UUID semantics to operate directly on byte[] and documents each type’s expected in-memory representation.
• Extends UuidUtils.toBytes(String) to accept both RFC-4122 strings and 32-char dashless hex, with tests covering round-trips.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pinot-spi/src/main/java/org/apache/pinot/spi/utils/UuidUtils.java	Adds dashless-hex parsing fallback for UUID string → bytes.
pinot-spi/src/test/java/org/apache/pinot/spi/utils/UuidUtilsTest.java	Adds test for dashless hex UUID round-trip and updates invalid test cases.
pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotDataType.java	Changes UUID external form to UUID (and arrays), and moves JSON→UUID parsing into JSON.toUUID.
pinot-spi/src/test/java/org/apache/pinot/spi/utils/PinotDataTypeTest.java	Updates tests to reflect new UUID external form and validates toInternal behavior.
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java	Aligns UUID values with byte[] stored type and updates equality/hash/compare/toString/convert behavior.

[xiangfu0]

Reviewed this as the more complete replacement for #18926 (which I closed). Overall LGTM — establishing byte[] as the uniform in-memory form for UUID (matching BYTES) and dropping the toBytesValue indirection is the right call, and it's cleaner than the narrower cleanup I had. A few things I verified and a few questions.

Verified ✔️

• Direct (byte[]) casts restore the original contract. equals/hashCode/compare/toString casting straight to byte[] isn't a new risk — it's exactly what BYTES did before [UUID 1/8] Add logical UUID type foundation (pinot-spi) #18869 (git show 92a0d98d8f^ → Arrays.equals((byte[]) value1, (byte[]) value2)). [UUID 1/8] Add logical UUID type foundation (pinot-spi) #18869 introduced the ByteArray-tolerant toBytesValue; this PR reverts to the established form and extends it to UUID.
• Value semantics unchanged for valid 16-byte values: Arrays.equals/Arrays.hashCode/ByteArray.compare are byte-identical to the old UuidUtils.* (same MSB/LSB unsigned ordering, same 31*h+b hash).
• @JsonIgnore on getDefaultNullValueString() is safe — SchemaSerializationTest/TableConfigsSerializationTest already assert it's never serialized (via @JsonValue + toJsonObject()), so this only reinforces the existing contract.
• toUuidBytesArray removal has no callers outside PinotDataType; STRING.toUUID keeps its .trim().

Nice catch worth a test

The compare fold moves BOOLEAN into the INT case. The old case BOOLEAN: Boolean.compare((boolean) value1, …) was latently broken — BOOLEAN values are held as Integer (convert → BooleanUtils.toInt, DEFAULT_..._BOOLEAN = 0), so that branch would ClassCastException if ever invoked. Integer.compare((int) value1, …) fixes it. Might be worth a one-line compare assertion for BOOLEAN (and TIMESTAMP) to pin the corrected dispatch.

Questions (non-blocking)

1. UuidUtils.toBytes(String) hex fallback widens UUID string parsing globally. The dashless 32-char hex form is now accepted everywhere toBytes(String)/toUUID(String)/isUuid(String) runs — ingestion, query literals, type inference — not just the byte[] default-null round-trip that motivates it. In particular isUuid("<32 hex chars>") now returns true, so any 32-hex string classifies as a UUID. Is making dashless hex a permanent first-class UUID input format intended, or should the tolerance be scoped to the default-null path? (The canonical-dashed strict check via equalsIgnoreCase is unaffected — this only adds the hex branch.)
2. getDefaultNullValueString() drops the _dataType != null guard and now NPEs if _dataType is unset. All current callers use fully-built specs so it's safe today; just flagging that the defensive branch is gone.

🤖 Automated review via Claude Code

[Jackie-Jiang]

Thanks for the thorough review!

Q1 (hex fallback scope): Keeping it global, intentionally. isUuid has no production callers (it's test-only), so there's no type-inference path that would misclassify an arbitrary 32-hex string as a UUID. The only production effect is that toBytes(String)/toUUID(String) accept the dashless hex rendering on inputs already declared as UUID (query literals, ingestion), which is a legitimate 16-byte rendering of a UUID; the strict canonical (equalsIgnoreCase) path is unchanged. Scoping it narrowly would mean reintroducing a UUID-specific stringification special-case in FieldSpec just to guard a low-risk path, so I'd rather keep the single tolerant toBytes(String).

Q2 (getDefaultNullValueString() guard): Left as-is. It's only ever called on fully-built specs (data type set), same assumption as the other value-handling methods on the enum, so I didn't want to add back a guard for a contract that already holds.

Test suggestion: Added FieldSpecTest#testCompare pinning the BOOLEAN -> INT and TIMESTAMP -> LONG compare dispatch. Pushed.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 66.66667% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.82%. Comparing base (9ceca0f) to head (4ca914c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...java/org/apache/pinot/spi/utils/PinotDataType.java	51.72%	14 Missing ⚠️
...main/java/org/apache/pinot/spi/data/FieldSpec.java	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #18927   +/-   ##
=========================================
  Coverage     64.81%   64.82%           
  Complexity     1347     1347           
=========================================
  Files          3396     3396           
  Lines        212503   212481   -22     
  Branches      33484    33475    -9     
=========================================
+ Hits         137732   137734    +2     
+ Misses        63610    63585   -25     
- Partials      11161    11162    +1     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (ø)
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-21	64.82% <66.66%> (+<0.01%)	⬆️
temurin	64.82% <66.66%> (+<0.01%)	⬆️
unittests	64.81% <66.66%> (+<0.01%)	⬆️
unittests1	56.79% <66.66%> (+0.01%)	⬆️
unittests2	37.22% <11.11%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.