VojtechMucha commented on PR #19035: URL: https://github.com/apache/pinot/pull/19035#issuecomment-5280719650
> **Minor / nits** > > * `var stringVal = getString(columnIndex)` in `PinotResultSet.parseJson` — `var` is essentially unused in this module (and Pinot main source broadly); prefer explicit `String stringVal = ...`. > * Per-cell `ColumnDataType.valueOf(name)` on every `getObject` re-parses an invariant — could resolve a `Map<Integer, ColumnDataType>` once at construction (small win, client path). > * `DriverUtils.isArrayDataType` uses exception-as-control-flow (`valueOf` in try/catch); optional cleanup via a static lookup map. > * The gRPC `getString` → `getValue` refactor is broader than the stated MAP/array scope — but it's a genuine improvement (old `.toString()` NPE'd on null cells). Worth a line in the PR description plus a null-scalar gRPC test. > * Pre-existing, out of scope: `getShort()` in both classes autounboxes a possibly-null `Integer` to `short` → NPE on SQL NULL. I addressed all the minors / nits. -- 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]
