xiangfu0 commented on PR #18927:
URL: https://github.com/apache/pinot/pull/18927#issuecomment-4899275073
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 #18869 (`git show 92a0d98d8f^`
→ `Arrays.equals((byte[]) value1, (byte[]) value2)`). #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](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
