morningman commented on issue #65423: URL: https://github.com/apache/doris/issues/65423#issuecomment-4933714668
Using this issue as the tracking issue for the Query Stats (column-level query/filter hit statistics) rework. Retitled accordingly. ## Background The feature was introduced in #18805 and recorded both column-level and tablet-level hits from the legacy planner, via `OriginalPlanner.collectQueryStat()` → `ScanNode.genQueryStats()`. When the legacy planner was deleted in #51056, that write path went away with it and nothing replaced it, so `show query stats` has returned `0` under Nereids ever since. That is the root cause of this report. ## Rebuild on Nereids - [x] #63067 — Part 1: scan slot registration, WHERE / JOIN `filterHit`, root-output `queryHit` - [x] #63768 — Part 2: GROUP BY / ORDER BY / window keys, `PhysicalRepeat`, `PhysicalPartitionTopN`, lazy materialization, `PhysicalStorageLayerAggregate`, alias propagation - [ ] #63974 — Part 3: UNION / INTERSECT / EXCEPT, CTE (inline and materialized), LATERAL VIEW / EXPLODE, HAVING on aggregate outputs, IN/EXISTS mark-join predicates, recursive CTE, computed SELECT expressions (#63061 was an earlier revision of Part 1, superseded by #63067.) ## Remaining work after Part 3 These came out of reviewing Part 3 against `master`. None of them is a regression introduced by the three PRs above — each is either older breakage or a coverage gap that Part 3 does not claim to close. Filing them here so the issue is not closed the moment Part 3 merges. - [ ] **Tablet-level statistics are dead code.** `QueryStats.addStats(List<Long> replicaIds)` has had no callers since #51056. The `QueryHits` column of `show tablets` / `show tablet <id>` / `ReplicasProcNode` is therefore permanently `0`, and unlike the rest of the feature it does not even respond to `enable_query_hit_stats`. Either restore the write path — under Nereids the selected replicas are only known after the Coordinator assigns fragments, so this is not a plan-time hook — or remove `TabletStats` along with the column and its three readers. A column that always reads zero is worse than no column. - [ ] **The HTTP endpoints do not match the SQL statements.** `SHOW QUERY STATS` merges counters from every live FE over thrift, and `CLEAN QUERY STATS` forwards to the Master and writes an editlog. But `GET /api/query_stats/...` reads only the local FE, and `DELETE /api/query_stats/...` calls `clear()` directly with no editlog (`QueryStatsAction.java`). On a multi-FE cluster the HTTP reads undercount and the HTTP deletes leave stale counters on every other frontend. - [ ] **Multi-input `HAVING` appears to be missed under two-phase aggregation.** `HAVING SUM(k2+k3) > 0` looks like it records `filterHit` only when the optimizer picks a one-phase aggregate. In a two-phase plan the local phase's alias lands only in Part 3's expansion map (never in `exprIdToScan`), while the global phase's `AggregateExpression` child is the partial buffer slot; since the map resolves a single hop, the chain breaks and the HAVING predicate resolves to nothing. The same reasoning applies to `SUM(DISTINCT k2+k3)` (where `NormalizeAggregate` hoists the argument into a bottom project) and to computed columns exposed through a materialized CTE. *This is derived from reading `SplitAggWithoutDistinct` / `NormalizeAggregate`, not yet reproduced with a live query* — the Part 3 unit test constructs a one-phase shape and the regression test only covers single-input `SUM(k2)`, so neither would catch it. Worth confirming with a `GROUP BY <non-bucket column> ... HAVING SUM(k2+ k3) > ...` regression case. - [ ] **Filters above set operations and window outputs look unattributed.** `SELECT * FROM (SELECT k1 FROM t1 UNION ALL SELECT k1 FROM t2) v WHERE k1 > 0`, and the QUALIFY pattern (`WHERE rn <= 10` around a `row_number()` subquery), both reference a freshly minted ExprId that maps to no scan, so `filterHit` should be lost. *Also from code reading rather than a live repro.* For the set-operation case the fix looks small: one output slot is fed by several branch slots, which is exactly the `Set<Slot>` shape Part 3's expansion map already carries. - [ ] External tables (Hive / Iceberg / JDBC / ODBC / ES) are out of scope by design — `walkPlan` recognizes only `PhysicalOlapScan`. The non-OLAP `TableStats.DEFAULT_INDEX_ID` branch still present in the read path is leftover from the original implementation and is unreachable today. Two properties worth documenting whatever we decide on the items above: 1. Every gap listed here is a **silent miss** — none of them produces a wrong count. So `FilterCount` should be read as a lower bound, not an exact value. A column with `FilterCount = 0` has not necessarily gone unfiltered, which matters because the main use of this feature is deciding prefix keys and where to build indexes. 2. Recording happens at **plan time** (`NereidsPlanner.plan()`), not at execution time. A query that plans but then fails or is cancelled is still counted, while a query served from the FE SQL cache is not counted at all (its physical plan is a `PhysicalSqlCache` with no scan nodes). The counter means "queries planned," not "queries executed." -- 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]
