Jackie-Jiang commented on PR #18927: URL: https://github.com/apache/pinot/pull/18927#issuecomment-4899379610
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. -- 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]
