yashmayya opened a new issue, #19145: URL: https://github.com/apache/pinot/issues/19145
## 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 makes the result depend on the plan that happens to be chosen: | Query | Non-scan plan | Scan plan (forced by adding `SUM`) | |---|---|---| | `MINLONG(doubleCol)` | ❌ `IllegalArgumentException` | ✅ returns `1` | | `MAXLONG(doubleCol)` | ❌ `IllegalArgumentException` | ✅ returns `10` | | `MINSTRING(intCol)` | ⚠️ silently returns `1` (`Integer`) | ❌ `BadQueryRequestException` | | `MAXSTRING(intCol)` | ⚠️ silently returns `10` (`Integer`) | ❌ `BadQueryRequestException` | So the two families fail in opposite directions: - **`MINLONG` / `MAXLONG` over `FLOAT` / `DOUBLE` / `BIG_DECIMAL`** — the non-scan path is *too strict*: it throws on a query the scan path computes fine. - **`MINSTRING` / `MAXSTRING` over a numeric column** — the non-scan path is *too lax*: it silently answers a query that `MinStringAggregationFunction` / `MaxStringAggregationFunction` explicitly reject as invalid, and the value it returns isn't `MINSTRING`/`MAXSTRING` semantics anyway. `MAXSTRING` is a lexicographic max, so over values `1..10` the answer would be `"9"`, not the `10` returned from the dictionary. The second one is the more concerning of the two, since it's a wrong result rather than an error. ## Root cause [`AggregationPlanNode#isFitForNonScanBasedPlan`](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L169-L195) gates eligibility on dictionary presence alone: ```java if (DICTIONARY_BASED_FUNCTIONS.contains(aggregationFunction.getType())) { if (dataSource.getDictionary() != null) { continue; // <- deemed resolvable; no column-type check } } ``` `DICTIONARY_BASED_FUNCTIONS` includes `MINLONG`, `MAXLONG`, `MINSTRING` and `MAXSTRING` ([L49-L56](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java#L49-L56)), but the resolvers in `NonScanBasedAggregationOperator` are stricter — or looser — than that gate: - `getMinValueLong` / `getMaxValueLong` require the stored type to be exactly `INT` or `LONG` and throw otherwise ([L202-L206](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java#L202-L206)). `FLOAT`/`DOUBLE`/`BIG_DECIMAL` pass the gate and then hit that `Preconditions.checkArgument`. - The `MINSTRING` / `MAXSTRING` cases return `dictionary.getMinVal()` / `getMaxVal()` unconverted ([L108-L122](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/query/NonScanBasedAggregationOperator.java#L108-L122)), bypassing the input validation those functions perform on the scan path (`BadQueryRequestException("Cannot compute MINSTRING for numeric column: ...")`, [MinStringAggregationFunction L61](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MinStringAggregationFunction.java#L61)). The result is a boxed number from a function declared `NullableSingleInputAggregationFunction<String, String>` with `ColumnDataType.STRING`. In short, the eligibility predicate and the resolvers disagree about what "resolvable" means. ## Steps to reproduce Offline table, 10 rows, two dictionary-encoded metric columns: `doubleCol = i + 0.5` and `intCol = i` for `i = 1..10`. ```sql -- IllegalArgumentException: MINLONG aggregation function can only be applied to columns of integer types SELECT MINLONG(doubleCol) FROM myTable; SELECT MAXLONG(doubleCol) FROM myTable; -- succeeds (returns 1): the extra SUM makes the query ineligible for the non-scan plan SELECT MINLONG(doubleCol), SUM(intCol) FROM myTable; -- silently returns 1 and 10 as Integers, from the dictionary SELECT MINSTRING(intCol) FROM myTable; SELECT MAXSTRING(intCol) FROM myTable; -- BadQueryRequestException: Cannot compute MINSTRING for numeric column: INT SELECT MINSTRING(intCol), SUM(intCol) FROM myTable; ``` Verified on `master` at `dc95530c8d` by running these through `InstancePlanMakerImplV2` against a generated segment; the first group is served by `NonScanBasedAggregationOperator` and the mixed queries by `AggregationOperator`. ## Expected behaviour A query should not change its answer — or flip between succeeding and failing — based on whether it qualifies for the non-scan plan. Specifically: - `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 produces the correct value or the correct error): - `MINLONG` / `MAXLONG`: require stored type `INT` or `LONG`. - `MINSTRING` / `MAXSTRING`: require stored type `STRING`. ## Related #18334 adds a numeric-type guard so that `MIN`/`MAX`/`MINMAXRANGE` on non-numeric columns are no longer resolved from metadata — the same shape of fix, but it doesn't cover these two families: it uses `isNumeric()`, which is not strict enough for `MINLONG`/`MAXLONG` (`DOUBLE` is numeric but not an integer type), and it doesn't touch `MINSTRING`/`MAXSTRING`. Worth flagging for whoever picks this up: that PR also extends metadata-based resolution to queries where only *some* aggregations are resolvable. That's a good change on its own, but it means the mixed queries above (`MINLONG(doubleCol), SUM(intCol)`) would resolve the typed MIN/MAX from metadata too — so the "add a scanned aggregation" escape hatch stops working and those queries would begin failing as well. Fixing the eligibility check covers both cases. -- 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]
