Jackie-Jiang opened a new pull request, #19334:
URL: https://github.com/apache/pinot/pull/19334
## Summary
Follow-up to #19332, where this was noticed and deliberately left out.
`FunnelEventsFunctionEvalAggregationFunction#getExtraFieldsBlocks` and both
switches in
`TimeSeriesAggregationFunction` chose their `BlockValSet` getter by the
**logical** value type. The getters are
keyed to the **stored** representation, so:
| Column | Stored as | Before | After |
|---|---|---|---|
| `BOOLEAN` | `INT` | rejected as unsupported | read via `getIntValuesSV()` |
| `TIMESTAMP` | `LONG` | needed a hand-written extra case | folds into
`LONG` |
| `JSON` | `STRING` | rejected as unsupported | read via
`getStringValuesSV()` |
The tell was a hand-written `case TIMESTAMP:` glued onto `case LONG:` in the
funnel switch. That case only existed
because the dispatch was on the wrong type — `BOOLEAN` and `JSON` never got
the same manual patch, so they still
failed. It is removed, since `TIMESTAMP` folds into `LONG` once the stored
type is used.
`TimeSeriesAggregationFunction` also gains the `FLOAT` case it was missing.
Its numeric path reads
`getDoubleValuesSV`, which works for every numeric stored type, so `FLOAT`
was excluded by omission rather than by
intent.
### What was checked and deliberately left alone
The distinction is whether the code is **choosing a getter** or **enforcing
a contract**:
- Choosing a getter must use the stored type — the three sites fixed here.
- Enforcing a contract must not. The sketch functions gate on logical
`BYTES` to mean *"this column holds a
serialized sketch"* rather than values to hash, which is the distinction
#18873 introduced; testing the stored
type there would misread a UUID column. `BaseBooleanAggregationFunction`
rejects a non-`BOOLEAN` column on
purpose, and using the stored type would wrongly accept plain `INT`.
- Every `dictionary.getValueType()` call already returns the stored type —
`IntDictionary` reports `INT`, so a
`BOOLEAN` column's dictionary reports `INT` — so those are correct as
written.
Swept for `switch` on a raw value type repo-wide and for equality
comparisons across the aggregation package;
these three are the complete set.
### Testing
`StoredTypeDispatchTest` is new and covers both functions, verified by
mutation — reverting either fix, or dropping
the `FLOAT` case, fails the corresponding test.
It excludes one case deliberately. For the time-series function a `JSON`
column now reaches
`aggregateStringValues`, but `BaseTimeSeriesBuilder` leaves string input
unimplemented and the `SUM` builder throws
on it. That limitation sits downstream of this change, so asserting on it
here would be testing the wrong
component. `JSON` is still covered on the funnel side, where all three types
work.
--
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]
