xiangfu0 commented on PR #19035:
URL: https://github.com/apache/pinot/pull/19035#issuecomment-5230137017
Thanks for adding this — MAP and array support in the JDBC driver is a
genuinely useful gap to close, and the implementation is clean: static
thread-safe `ObjectReader` reuse, pre-sized lists with plain loops, correct
`JAVA_OBJECT`/`List`/`Map` metadata, and
`ClassCastException`→`SQLDataException` guards that I verified are safe against
both the JSON and Arrow gRPC encoders. The gRPC `getString` refactor also
incidentally fixes a latent NPE on null cells.
**Approve with changes** — no hard blockers, but I'd prioritize three
correctness/contract items before merge (details inline):
### Major
1. **`UUID_ARRAY` throws a self-contradictory error.**
`UUID_ARRAY.isArray()` is `true`, so `getObject` routes it to `getList`, which
has no case for it and throws `"Data type is not an array: UUID_ARRAY"` — for a
type that *is* an array. Meanwhile `DriverUtils.isArrayDataType("UUID_ARRAY")`
returns `true`, so the metadata advertises `List`. Metadata and accessor
disagree. Affects both paths.
2. **HTTP vs gRPC MAP return different Java value types.** HTTP binds via
Jackson `TypeReference<Map<?,?>>` (→ `Integer`/`Long`/`Double`), while gRPC
wraps the decoder's map whose values come from
`JsonResponseEncoder.extractValue(JsonNode)` and can be
`Short`/`BigInteger`/`Float`/`BigDecimal`. Same query, transport-dependent
types — breaks `equals()` and downstream numeric handling.
3. **`getObject` still throws for scalar `BIG_DECIMAL`/`TIMESTAMP`/`JSON`**
while `DriverUtils` advertises them. This is *pre-existing* (the old switch
behaved the same), so not a regression — but the PR is rebuilding this exact
switch and could close it cheaply.
4. **Conversion helpers are duplicated** across the two result sets
(`getTimestampList`/`getBytesList` vs
`toTimestampList`/`toBytesList`/`toBigDecimalList`) — risk of drift; worth
extracting shared helpers.
5. **Test-coverage gaps:** MAP is untested on the HTTP path; there are no
negative/error-path tests (bad hex/timestamp/decimal, malformed JSON, unknown
type, gRPC non-Map value); and gRPC never tests a null array/map or asserts
`wasNull()`.
6. **Nullable-returning helpers lack `@Nullable`**
(`getMap`/`getList`/`parseJson`/`getValue` etc.) — project convention is
`javax.annotation.Nullable`.
### Minor / nits
- `var stringVal` in `parseJson` is non-idiomatic for this module (use
explicit `String`).
- Per-cell `ColumnDataType.valueOf(name)` re-parses an invariant on every
`getObject` — could resolve `Map<Integer, ColumnDataType>` once at construction.
- `getList` default message should read `"Unsupported array data type: ..."`.
- The gRPC `getString`→`getValue` refactor is broader than the stated scope
(and a good fix) — worth a note in the PR description plus a null-scalar gRPC
test.
- Pre-existing, out of scope: `getShort()` in both classes NPEs on SQL NULL
(unboxes a null `Integer`).
Nice work overall — happy to re-review once the correctness items are
addressed.
--
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]
