xiangfu0 commented on code in PR #18873:
URL: https://github.com/apache/pinot/pull/18873#discussion_r3744737139
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java:
##########
@@ -531,6 +531,13 @@ private Object getConvertedKey(DataTable dataTable,
ColumnDataType columnDataTyp
return dataTable.getString(rowId, colId);
case BYTES:
return dataTable.getBytes(rowId, colId).getBytes();
+ case UUID:
+ // Deliberately delegated to ColumnDataType#convert rather than
falling through to BYTES. The other reduce
+ // path (reduceWithIndexedTable) converts group keys with exactly that
method, and UUID is the one type
+ // whose converted form is not its stored bytes -- it yields a
java.util.UUID. PredicateRowMatcher casts
+ // directly on that, so returning the raw byte[] here makes GROUP BY
... HAVING on a UUID column throw
+ // ClassCastException. Delegating keeps the two paths identical by
construction.
+ return columnDataType.convert(dataTable.getBytes(rowId, colId));
Review Comment:
I would say no, by the same rule we applied elsewhere in this PR:
`DataTable` has exactly one accessor per **stored** type.
```
getInt getLong getFloat getDouble getBigDecimal getString getBytes
```
There is no `getTimestamp` and no `getBoolean` — TIMESTAMP reduces to LONG
and BOOLEAN to INT, so callers read the stored type and convert, which is
literally the line above this one:
```java
case TIMESTAMP:
return new Timestamp(dataTable.getLong(rowId, colId));
```
`getBigDecimal` exists only because BIG_DECIMAL *is* its own stored type, so
the DataTable genuinely holds a `BigDecimal` — it is not a counterexample.
UUID reduces to BYTES, so `getBytes(...)` plus a conversion is the
consistent shape, and `getUuid()` would be the first accessor on this interface
for a logical type that is not a stored type. It would also have to be
implemented across every DataTable version on a wire-format-adjacent interface,
for a caller that already has `ColumnDataType#convert` doing the work.
Happy to add it if you would rather the reducer read `getUuid(...)`
directly, but it would be a new precedent rather than following the TIMESTAMP
one.
--
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]
