Metadata/dictionary based aggregation ignores column-type preconditions for MINLONG/MAXLONG and MINSTRING/MAXSTRING
- Dominant language
- Java
- Stars
- 6.1k
- Forks
- 1.5k
- Avg merge
- 2d 55m
- Merged PRs (30d)
- 182
Description
## Summary
The non-scan (metadata/dictionary based) aggregation path treats an aggregation as resolvable if the column merely **has a dictionary**, without checking that the column type is one the function actually supports. For the typed MIN/MAX variants this produces two failures in opposite directions:
- **`MINLONG` / `MAXLONG` over `FLOAT` / `DOUBLE` / `BIG_DECIMAL`** — the resolver is *stricter* than the eligibility check, so the query fails outright with `IllegalArgumentException`, even though the scan path computes it fine.
- **`MINSTRING` / `MAXSTRING` over a numeric column** — the resolver is *looser*, so it silently returns the raw dictionary min/max (a boxed number) for a query that `Min/MaxStringAggregationFunction` explicitly reject as invalid. The value returned isn't `MINSTRING`/`MAXSTRING` semantics either: these are lexicographic, so over values `1..10` the max would be `"9"`, not the `10` that comes back from the dictionary.
Current behaviour on `master` (verified at `9d6545bde1`), for a table with dictionary-encoded `doubleCol = i + 0.5` and `intCol = i`, `i = 1..10`:
| Query | Operator | Result |
|---|---|---|
| `SELECT MINLONG(doubleCol)` | `NonScanBasedAggregationOperator` | ❌ `IllegalArgumentException: MINLONG aggregation function can only be applied to columns of integer types` |
| `SELECT MAXLONG(doubleCol)` | `NonScanBasedAggregationOperator` | ❌ `IllegalArgumentException` |
| `SELECT MINLONG(doubleCol), SUM(intCol)` | `AggregationOperator` (partial) | ❌ `IllegalArgumentException` |
| `SELECT MINSTRING(intCol)` | `NonScanBasedAggregationOperator` | ⚠️ silently returns `1` (`Integer`) |
| `SELECT MAXSTRING(intCol)` | `NonScanBasedAggregationOperator` | ⚠️ silently returns `10` (`Integer`) |
| `SELECT MINSTRING(intCol), SUM(intCol)` | `AggregationOperator` (partial) | ⚠️ silently returns `1` (`Integer`) |
## Root cause
[`AggregationPlanNode#isFitForNonScanBasedPlan`](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L205-L232) treats any dictionary-encoded column as resolvable for the functions in `DICTIONARY_BASED_FUNCTIONS`:
```java
if (dataSource.getDictionary() != null && DICTIONARY_BASED_FUNCTIONS.contains(functionType)) {
return true; // <- no column-type check
}
```
That set includes `MINLONG`, `MAXLONG`, `MINSTRING` and `MAXSTRING` ([L50-L56](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L50-L56)), but the resolvers in `AggregationFunctionUtils#getAggregationResult` have different requirements:
- [`getMinValueLong` / `getMaxValueLong`](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java#L642-L672) require the stored type to be exactly `INT` or `LONG` and throw otherwise. `FLOAT`/`DOUBLE`/`BIG_DECIMAL` pass the eligibility check and then hit that `Preconditions.checkArgument`.
- The [`MINSTRING` / `MAXSTRING` cases](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java#L550-L564) return `dictionary.getMinVal()` / `getMaxVal()` unconverted, bypassing the validation those functions perform on the scan path ([`BadQueryRequestException("Cannot compute MINSTRING for numeric column: ...")`](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MinStringAggregationFunction.java#L61)). `MinStringAggregationFunction` is declared `NullableSingleInputAggregationFunction` with `ColumnDataType.STRING`.
There is already a guard of exactly the right shape immediately above, added by #18334 for `MIN`/`MAX`/`MINMAXRANGE` ([L64-L68](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L64-L68), [L219-L223](https://github.com/apache/pinot/blob/9d6545bde17d311129c1e52c3ff26d80f08fcb71/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L219-L223)) — it just uses `isNumeric()`, which is not strict enough for `MINLONG`/`MAXLONG` (`DOUBLE` is numeric but not an integer type) and doesn't cover `MINSTRING`/`MAXSTRING` at all.
## What changed with #18334
#18334 (merged as 9d6545bde1) extended metadata-based resolution to queries where only *some* aggregations are resolvable. That's a good change, but because the eligibility check doesn't encode these type preconditions, it also removed the workaround that previously masked this bug. Before that PR, adding any scanned aggregation forced the whole query onto the scan path; now the typed MIN/MAX is resolved from metadata regardless. Verified pre-merge at `dc95530c8d` vs post-merge at `9d6545bde1`:
| Query | Before #18334 | After #18334 |
|---|---|---|
| `SELECT MINLONG(doubleCol), SUM(intCol)` | ✅ returns `1` | ❌ `IllegalArgumentException` |
| `SELECT MINSTRING(intCol), SUM(intCol)` | ❌ `BadQueryRequestException` (correctly rejected) | ⚠️ silently returns `1` (`Integer`) |
So mixed queries went from working to failing in the first case, and from correctly rejected to silently wrong in the second. Single-aggregation queries behaved this way before #18334 as well — that part is long-standing, not a regression.
## Expected behaviour
A query shouldn't change its answer, or flip between succeeding and failing, based on whether it qualifies for the non-scan plan:
- `MINLONG`/`MAXLONG` over a non-integer numeric column should return the same value the scan path returns.
- `MINSTRING`/`MAXSTRING` over a numeric column should be rejected with the same `BadQueryRequestException` the scan path raises, not answered from the dictionary.
## Suggested fix
Make the eligibility check encode the resolvers' real type preconditions, so unsupported combinations fall back to the scan path (which already yields the correct value or the correct error):
- `MINLONG` / `MAXLONG`: require stored type `INT` or `LONG`.
- `MINSTRING` / `MAXSTRING`: require stored type `STRING`.
## Reproduction
```sql
SELECT MINLONG(doubleCol) FROM myTable;
SELECT MAXLONG(doubleCol) FROM myTable;
SELECT MINLONG(doubleCol), SUM(intCol) FROM myTable;
SELECT MINSTRING(intCol) FROM myTable;
SELECT MAXSTRING(intCol) FROM myTable;
SELECT MINSTRING(intCol), SUM(intCol) FROM myTable;
```
Offline table, 10 rows, two dictionary-encoded metric columns: `doubleCol = i + 0.5`, `intCol = i` for `i = 1..10`. Run through `InstancePlanMakerImplV2` against a generated segment.
Contributor guide
Research direction
Start in pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java at isFitForNonScanBasedPlan, then compare the typed requirements in AggregationFunctionUtils#getAggregationResult and MinStringAggregationFunction.java. Reproduce the listed queries through InstancePlanMakerImplV2 against a generated segment. Done means unsupported typed combinations use the scan path and match its value or BadQueryRequestException behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100