xiangfu0 commented on PR #19986: URL: https://github.com/apache/pinot/pull/19986#issuecomment-6373131789
## Code review summary Reviewed at head `ca99413` across correctness/null-handling, type inference (single- and multi-stage planner rule), backward-compat/serialization, and test coverage. Overall this is a clean, well-structured change that closely follows existing patterns. **No CRITICAL or MAJOR issues found.** Highlights of what I verified: - **Serde alignment:** `BigDecimalLongPair(55)` / `BytesLongPair(56)` are appended to `ObjectType` and the `SER_DES` array in matching order; 55/56 were unused, and the array is indexed directly by the enum value, so cross-node deserialization stays correct. - **`isSerializedPairInput()` refactor:** All three dispatch sites in both base classes are converted to the hook, and the new BYTES/UUID functions override it to `false` so a raw BYTES column is never misread as a serialized pair. The reverse is impossible for the other kernels. - **Planner rule:** termination is sound (appending the 3rd arg makes the next `matches()` return false), appended literals keep group-set/arg indexes valid, literal dedup per data type is safe, and `input` vs `unboxRel` row types are consistent. - **Type inference parity:** the multi-stage switch, the single-stage `addDataTypeArgument` switch, and `INFERRED_DATA_TYPES` all agree (incl. JSON→STRING, BOOLEAN, TIMESTAMP, DECIMAL→BIG_DECIMAL). The 2-arg operand checker correctly rejects multi-value/unsupported data columns. - **Back-compat:** the broker already only ever sent 3-arg typed calls to servers (2-arg never worked), so already-supported types are purely additive; the new TIMESTAMP/BIG_DECIMAL/BYTES/UUID types needing upgraded servers is already called out in the PR description. Only a few **MINOR** notes below — none blocking. Nice work, and thanks for the thorough tests. -- 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]
