xiangfu0 commented on PR #19035: URL: https://github.com/apache/pinot/pull/19035#issuecomment-5306576986
## Re-review of the updated PR Thanks for the thorough revision — this addresses **all six** items from the previous round: - ✅ `UUID_ARRAY` now handled (`getUuidList`/`toUuidList`) + tested, and `UUID` scalar added (`Types.OTHER`/`java.util.UUID`) - ✅ HTTP↔gRPC MAP value-type divergence fixed — gRPC now re-binds through the same untyped `MAP_READER`, with mixed-numeric round-trip tests on both paths - ✅ Scalar `BIG_DECIMAL`/`TIMESTAMP`/`JSON` now returned by `getObject` + tested - ✅ Conversion helpers de-duplicated into `AbstractBaseResultSet` (`toTimestampList`/`toBytesList`/`toUuidList`) - ✅ Negative/error-path, null/`wasNull`, and HTTP-MAP tests all added - ✅ `@Nullable` applied throughout Nice structural cleanup pulling the shared `getObject`/scalar/array logic into the base class. A few items from the refactor itself: ### Major **1. New `abstract` methods widen the contract of a published class.** `AbstractBaseResultSet` is a bare `public abstract class` in the released `org.apache.pinot.client.base` package (no `@InterfaceStability.Internal` / internal-only Javadoc). Adding `getMap`, `getColumnType`, and `getList` as `abstract` is a source-incompatible change — any external subclass will fail to compile against this version. (Demoting `validateState()` from abstract to concrete is the compatible direction and is fine.) *Options:* give the three new methods default implementations (`getColumnType` → `null`; `getMap`/`getList` → throw `SQLFeatureNotSupportedException`) so existing subclasses keep compiling; **or** mark the class `@InterfaceStability.Internal` / document it as not a supported extension point, and note the source-incompatibility in the PR description / release notes. ### Minor - **gRPC empty result set regression** — `getColumnType` does `_dataSchema.getColumnDataType(columnIndex - 1)`, but the `empty()` constructor leaves `_dataSchema == null`, so `getObject(...)` now NPEs where the old code threw a descriptive `SQLDataException`. Guard `_dataSchema == null` (or call `validateState()` first). - **`@Nullable` ordering** — KB C7.6 says `@Nullable` goes *above* `@Override`; the new annotations are ordered `@Override` then `@Nullable` (~12 sites across the three files). Mechanical fix. - **`@Nullable` consistency** — the abstract `getColumnType(int)` and `getList(int, ColumnDataType)` lack `@Nullable`, while both overrides (and the sibling abstract `getMap`) have it and can return null. - **`ColumnDataType.get(String)` naming** — the enum's existing static factories are `fromDataType*`; `get(String)` is generic and, unlike `valueOf`, returns `null` on unknown names (also differs from the existing `parseColumnDataType`, which throws a mixed-version-aware error). Consider `forName`/`getByName`. `isArray(String)` is fine. - **gRPC `getMap` allocates a throwaway String** via `MAP_READER.readValue(JsonUtils.objectToString(value))`; `objectMapper.convertValue(value, ...)` avoids the string intermediate. Client-side, MAP-only. - **Untested paths:** the moved `getShort(int)` → `(short) getInt(...)` truncation (add in-range + overflow cases), and `getObject(int, Class)` on a null cell (`assertNull`). - **Pre-existing (not introduced here):** `getScalar` returns boxed `0`/`0L`/`false` for a SQL-NULL numeric/boolean scalar rather than `null` per the JDBC contract — now visibly inconsistent with the null-array path that correctly returns null; and `getAsciiStream`/`getUnicodeStream`/`getCharacterStream` call the now-`@Nullable` `getString(...)` with no null guard → NPE on a NULL cell. Worth a follow-up while this code is being touched. Net: no blockers. The one item I'd resolve before merge is the widened abstract contract on the published `AbstractBaseResultSet` (default methods, or an explicit internal-only marker + release note). The rest are quick mechanical fixes. Looks close. -- 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]
