xiangfu0 commented on PR #18681: URL: https://github.com/apache/pinot/pull/18681#issuecomment-5200621885
Follow-up review of `d982480aa2d1a012e8f53b411f79e7eebd3c8797`, plus the three suppressed comments from the latest Copilot pass. ### Blocking defect found and fixed `buildResult` rewrites every user GROUP BY key to the MV column holding it, without checking for a lookup miss. When the MV groups by a key it never projects, the rewritten query carried `GROUP BY <null identifier>` and failed on the server instead of falling back to the base table. Treating a scalar as a direct projection hit newly exposed this path. Reproduced by running the same probe against this branch and against the merge base `c5f7bda`: ```text MV : SELECT UPPER(city) AS uc, SUM(revenue) AS sum_rev FROM orders GROUP BY city USER : SELECT UPPER(city), SUM(revenue) FROM orders GROUP BY city c5f7bda -> NO MATCH d982480 -> MATCH cost=6.0, groupByList:[Expression(type:IDENTIFIER, identifier:Identifier(name:null))] ``` The underlying hole predates this PR — the same `Identifier(name:null)` is produced on `c5f7bda` by an MV grouped by a plain identifier it does not project (`SELECT SUM(revenue) AS sum_rev FROM orders GROUP BY city`) — but this PR widens its reach, so the guard belongs here. Fix: `groupByMatches` now receives the projection map (matching `orderByCompatible` / `havingCompatible`, which already take it) and rejects any candidate whose user GROUP BY keys are not all materialized. Whole-table re-aggregation is unaffected, since it remaps no key. Four regressions added: the scalar rejection, the plain-identifier rejection, the positive case once the key is also projected, and the whole-table path that must keep matching. ### Suppressed comment 1 — `IllegalStateException` in `buildReAggSelectList` Keeping it, deliberately. It is unreachable: `projectionSubsumes` enforces the identical `!isAggregate && containsKey` condition against the same map before `buildResult` runs. It also matches the module's stated convention — see the comment above `strategy.match` in `MaterializedViewQueryRewriteEngine.tryRewrite`, which says a thrown exception is a strategy bug that should surface rather than be swallowed, and notes that `BaseSingleStageBrokerRequestHandler` already wraps the rewrite so the query still falls back. The sibling `rewriteAggregationExpression` throws the same way. ### Suppressed comment 2 — exact float cost assertions Fixed. Both residual tests now assert `MatchType.AGG_REAGG` instead of `getCost() == 7.0`. The residual-specific coverage is unchanged: each still asserts the filter is retained on the MV side and remapped to the MV column name (`day`, `mv_city`). ### Suppressed comment 3 — `LIMIT 10000` seeding the scalar MV segment Fixed. The limit is now the named constant `MATERIALIZED_VIEW_SOURCE_ROW_LIMIT`, documented as the reason it matters, and the setup asserts the returned row count is strictly below it. A dataset that outgrows the cap now fails loudly instead of silently materializing a prefix and validating a truncated view. ### Validation (JDK 25, local) - `pinot-materialized-view`: 356 tests passed (`AggregationSubsumptionStrategyTest` 52, up from 48) - `MaterializedViewClusterIntegrationTest`: 12 passed - Spotless, Checkstyle, license format/check: clean on both affected modules - Warning-enabled `test-compile`: no warnings in the changed files -- 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]
