VojtechMucha commented on PR #19035: URL: https://github.com/apache/pinot/pull/19035#issuecomment-5280692071
> **Major — duplicated conversion helpers + missing `@Nullable`** (`PinotResultSet` vs `PinotGrpcResultSet`) > > `getTimestampList`/`getBytesList` (HTTP) and `toTimestampList`/`toBytesList`/`toBigDecimalList` (gRPC) are near-identical: same sizing, shared `Calendar`, same null guard, same exception wrapping/message. This risks the two drivers drifting. Consider extracting shared `parseTimestamps`/`parseBytes`/`parseBigDecimals(List<String>)` helpers (e.g. in `client/utils`, next to `DateTimeUtils`) with both result sets normalizing raw values to strings and delegating. > > Separately: the new helpers (`getMap`/`getList`/`parseJson`/`getValue`, etc.) return `null` as a first-class outcome but carry no `@Nullable`. Please add `javax.annotation.Nullable` per project convention — `getValue` in particular has the `_wasNull` side effect worth documenting. I moved common logic to `AbstractBaseResultSet`. `Nullable` annotation used now in `AbstractBaseResultSet`, `PinotResultSet`, `PinotGrpcResultSet`. -- 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]
