akashchamp opened a new pull request, #19653:
URL: https://github.com/apache/pinot/pull/19653
## Description
Fixes #19603.
`SELECT MIN(col), MAX(col) FROM table` on a `STRING`-typed column (e.g. a
`dateTimeFieldSpec` with
`SIMPLE_DATE_FORMAT`, or a plain `STRING` dimension) fails with a bare
`NumberFormatException`
(`For input string: "2013-07-01"`), even though the column is declared
correctly. The error gives no
indication that Pinot already has `MINSTRING`/`MAXSTRING`, or that
`MIN`/`MAX` can be automatically
rewritten to them via the `autoRewriteAggregationType=true` query option
(added in #16980 / #17058).
This PR implements suggestion 1 from the issue: **improve the error
message**, without changing any
query behavior or defaults (no breaking changes).
### Root cause
`MIN`/`MAX` are numeric-only in Pinot (`AggregationFunctionType.MIN`/`MAX`
map to `SqlTypeName.DOUBLE`).
Two separate code paths convert the resolved value to a `double` without
checking the column's type
first, so a `STRING` value falls through to `Double.parseDouble(...)` and
throws an opaque
`NumberFormatException`:
1. **Non-scan / dictionary-metadata path**
(`NonScanBasedAggregationOperator`, used for e.g.
`SELECT MIN(col) FROM t` with no filter):
`AggregationFunctionUtils#toDouble`.
2. **Scan-based group-by path** (`GroupByOperator`, used for e.g. `... GROUP
BY ...`):
`MinAggregationFunction`/`MaxAggregationFunction#aggregateSVGroupBySV`
(and the MV group-by
variants), which — unlike `aggregateSV`, which already validates the
stored type — called
`BlockValSet#getDoubleValuesSV/MV` directly.
### Changes
- `AggregationFunctionUtils#toDouble`: catches the `NumberFormatException`
and rethrows a
`BadQueryRequestException` naming `MINSTRING`/`MAXSTRING` and
`autoRewriteAggregationType`.
- `MinAggregationFunction`/`MaxAggregationFunction`: added a
`checkNumericType` guard, called from
`aggregate()`, `aggregateGroupBySV()`, and `aggregateGroupByMV()`
(mirroring the existing check
already present in `aggregateSV()`), so every entry point fails fast with
the same clear message
instead of only the plain scalar SV path being covered. The pre-existing
message in `aggregateSV()`'s
`default` case was extended with the same guidance for consistency.
No behavior changes for numeric columns; `MINSTRING`/`MAXSTRING` and
`autoRewriteAggregationType` are
unchanged.
Documenting the workaround on the `MIN`/`MAX` docs page (suggestion 2) and
defaulting
`autoRewriteAggregationType` to `true` (suggestion 3, a behavior change
needing separate discussion)
are left out of scope for this PR.
## How was this tested?
- Added unit tests reproducing both reported stack traces and asserting the
new message:
- `AggregationFunctionUtilsTest#testMinAndMaxOnNonNumericDictionaryThrows`
— dictionary-encoded
non-numeric value (the `NonScanBasedAggregationOperator` path from the
issue).
-
`MinAggregationFunctionTest`/`MaxAggregationFunctionTest#testNonNumericColumnExceptionIn{Aggregate,
AggregateGroupBySV,AggregateGroupByMV}Method` — `STRING` `BlockValSet`,
mirroring the existing
`MinStringAggregationFunctionTest` pattern for the reverse
(numeric-on-*STRING-only* function) case.
- `./mvnw -pl pinot-core -am test
-Dtest=AggregationFunctionUtilsTest,MinAggregationFunctionTest,MaxAggregationFunctionTest
-Dsurefire.failIfNoSpecifiedTests=false test`
- `./mvnw -pl pinot-core spotless:apply license:format checkstyle:check
license:check`
- Manually verified against a local table with a `STRING` column that both
queries below now return
a `BadQueryRequestException` whose message points to
`MINSTRING`/`MAXSTRING` and
`autoRewriteAggregationType`, instead of a raw `NumberFormatException`:
- `SELECT MIN(stringCol) FROM t` (no `GROUP BY`, non-scan path)
- `SELECT dimCol, MIN(stringCol) FROM t GROUP BY dimCol` (scan-based
group-by path)
## Notes for reviewers
I don't have write access to add labels to this PR — happy to see the
appropriate label
(`bugfix`) applied.
This change was prepared with AI assistance (Claude Code).
--
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]
