morningman commented on PR #63067: URL: https://github.com/apache/doris/pull/63067#issuecomment-4491061663
# Improvement Suggestions for PR #63067 **PR**: [[Fix](Query Stats) Add QueryStatsRecorder for column-level query and filter - Part1](https://github.com/apache/doris/pull/63067) **Review Date**: 2026-05-19 --- ## 1. [High] Rework the SELECT-column capture: walk the whole plan instead of `plan.getOutput()` The current implementation only records a `queryHit` for a column if that column's `ExprId` survives all the way through to the root plan's `getOutput()`. In practice, this misses several common cases: - `SELECT k1 AS x FROM t` — `PhysicalProject` assigns a new `ExprId` to `x`, so `k1` is not matched. - `SELECT COUNT(k1) FROM t` — the output slot is the aggregate result; `k1` never appears in the output `ExprId` set. - `SELECT k1 + 1 FROM t` — the output slot is an expression result; `k1` is not matched. - `SELECT k1 FROM (SELECT k1 FROM t) sub` — intermediate `Project` nodes may also rewrite the `ExprId`. - Union / window / group-by / order-by columns that don't reach the root output are silently dropped. The PR description already calls out group-by/order-by/window as future work, but **plain `SELECT alias` and `SELECT expr(col)` are also missed today**, which is narrower than the description implies. Two options: 1. **Preferred**: walk the entire plan tree and, for each node, scan every `Expression` in the node (projections, agg keys, sort keys, join conditions, window expressions, etc.) for `SlotReference`s. Each `SlotReference` whose `ExprId` matches a registered scan contributes a `queryHit` for the corresponding column. `PhysicalFilter` conjuncts continue to contribute `filterHit`. Sketch: ```java private static void walkPlan(Plan plan, Map<ExprId, PhysicalOlapScan> exprIdToScan, Map<String, StatsDelta> deltas) { if (plan instanceof PhysicalOlapScan) { ... return; } if (plan instanceof PhysicalDeferMaterializeOlapScan) { ... return; } for (Plan child : plan.children()) { walkPlan(child, exprIdToScan, deltas); } if (plan instanceof PhysicalFilter) { for (Expression conjunct : ((PhysicalFilter<?>) plan).getConjuncts()) { recordSlots(conjunct.getInputSlots(), exprIdToScan, deltas, /*filter=*/true); } } plan.getExpressions().forEach(expr -> recordSlots(expr.getInputSlots(), exprIdToScan, deltas, /*filter=*/false)); } ``` 2. **Minimum**: if you'd rather keep Part 1 small, at least update the class JavaDoc to spell out that the SELECT side only matches base columns that flow straight through to the root plan's output and that aliased/projected/aggregated columns are deliberately not recorded yet. ## 2. [High] Add unit tests for the main `walkPlan` paths `QueryStatsRecorderTest` covers only the `shouldRecord` guard branches. The actual recording logic — the part that writes data into `QueryStats` — has no unit-level coverage and is only exercised indirectly via the regression suite. Suggested additions: 1. `Filter(k2=1) → Scan(k1,k2)`: assert that the resulting delta marks `k2.filterHit=true` and `k1.filterHit=false`. 2. `Scan(k1,k2)` as the root plan: assert `k1.queryHit=true` for output columns. 3. `Filter → Filter → Scan` with overlapping conjuncts: assert each column is recorded exactly once and that re-running `record()` is idempotent (covered by `queryStatsRecorded`). 4. `PhysicalDeferMaterializeOlapScan` wrapping a `PhysicalOlapScan`: assert the inner scan is the one keyed in `exprIdToScan` and the delta is built against the inner table's identifiers. Mockito can stub `getOutput()`, `getConjuncts()`, `getTable()`, `getDatabase()`, and `getSelectedIndexId()` directly — no FE bring-up required. Also note: `testShouldNotRecordInternalQuery` mocks `ConnectContext`, which carries final/thread-local state in some places. Run the test once locally to make sure Mockito's default configuration can stub `getState()` (you may need `mockito-inline`). ## 3. [Medium] Treat join conditions as filter hits Right now only `PhysicalFilter.getConjuncts()` is mined for filter columns. Users naturally think of join keys (`a.k = b.k`) and other join predicates as "filters" on a column. The most-impactful Part 2 candidate is to also walk: - `PhysicalHashJoin.getHashJoinConjuncts()` - `PhysicalHashJoin.getOtherJoinConjuncts()` - `PhysicalNestedLoopJoin` predicates - Any mark-join or anti-join condition If suggestion 1 is implemented as a generic expression walker, join conditions naturally fall out of the same pass. ## 4. [Medium] Drop `LinkedHashMap` for `deltas` ```java Map<ExprId, PhysicalOlapScan> exprIdToScan = new HashMap<>(); Map<String, StatsDelta> deltas = new LinkedHashMap<>(); ``` The iteration order over `deltas` has no externally observable effect (`addStats` aggregates into a `ConcurrentHashMap` keyed by catalog id), so `LinkedHashMap` is unnecessary and gives readers the impression that insertion order matters here. Use `HashMap`. The composite `String` key (`catalogId + "_" + dbId + "_" + tableId + "_" + indexId`) is fine for correctness; it does produce a transient `String` per query. A small `record`/tuple class would be tidier, but this is a follow-up nicety, not a blocker. ## 5. [Medium] Make the warn-log actionable ```java LOG.warn("Failed to record query stats", e); ``` There's no query id, table, or SQL context. If statistics recording fails repeatedly for one shape of query, this line floods `fe.log` with stack traces that can't be tied back to the trigger. At minimum, include the query id from `connectContext.queryId()`. If a particular plan shape is going to fail on every call, consider rate-limiting (one warn per N seconds, or sampled) so the log stays readable. ## 6. [Medium] Document the scope on the class The class JavaDoc is currently a one-liner. Future readers (and operators interpreting `SHOW QUERY STATS`) need to know: - Only `OlapTable` is recorded — external tables (Hive, Iceberg, JDBC, ES, …) are not. - The SELECT side only matches base columns that survive to the root plan's output. Aliased / expression / aggregated projections are not recorded yet. - DML, internal queries (e.g. auto-analyze), and `EXPLAIN` are skipped. - Per query, each table's query count is incremented at most once, regardless of how many times the table is scanned. Spelling this out reduces support load and gives Part 2/3 a clear starting contract. ## 7. [Low] Open up `addStats` per-table, not per-query ```java for (StatsDelta delta : deltas.values()) { if (!delta.empty()) { Env.getCurrentEnv().getQueryStats().addStats(delta); } } ``` This entire loop sits inside the outer `try`. If one table's `addStats` throws (it declares `AnalysisException`), the remaining tables aren't recorded for this query. Pushing the try/catch down to per-delta lets the other tables still get credit. The trade-off is that local failures become quieter — weigh which behavior you want. ## 8. [Low] Rename `setQueryStatsRecorded()` ```java public void setQueryStatsRecorded() { queryStatsRecorded = true; } ``` A setter with no argument reads as one half of a setter/getter pair. The intent is "set this latch, once". Either: - Rename to `markQueryStatsRecorded()` to make the monotonic latch semantics obvious; or - Keep `set...` but accept `boolean` so it pairs symmetrically with `isQueryStatsRecorded()`. ## 9. [Low] Simplify `unwrapSlotRef` ```java private static SlotReference unwrapSlotRef(Expression expr) { if (expr instanceof SlotReference) { return (SlotReference) expr; } if (expr instanceof Alias) { return unwrapSlotRef(((Alias) expr).child()); } return null; } ``` `plan.getOutput()` always returns `Slot`s, and a physical plan's output is exclusively `SlotReference` — `Alias` is a logical expression and won't appear here. The `Alias` branch is unreachable for the only caller. Two reasonable directions: - Tighten to `return expr instanceof SlotReference ? (SlotReference) expr : null;` and drop the helper; or - Keep the helper but add a comment explaining it is defensive for hypothetical callers that pass projection `Alias`es directly. If you adopt suggestion 1, this helper should grow into a full `Expression`-tree walker that surfaces every `SlotReference`, rather than peeling a single `Alias`. ## 10. [Low] `if (!col.getName().isEmpty())` is dead defensive code `Column` objects owned by an `OlapTable` always carry a non-empty name. The two guards in `record()` and `walkPlan()` can be dropped, or replaced with `Preconditions.checkState(!col.getName().isEmpty())` so a real violation is loud rather than silent. ## 11. [Low] Restore session variables safely in the regression test ```groovy sql "set enable_nereids_planner = false" sql "set enable_query_cache = true" ``` These end-of-suite resets assume the connection entered the suite with `enable_nereids_planner=false` and `enable_query_cache=true`. If the connection is reused by another suite that expects different defaults, you'll leak state. Two safer patterns: - Capture the entry value, then restore it: `def origCache = sql("select @@enable_query_cache")[0][0]; ... ; sql "set enable_query_cache = ${origCache}"`. - Wrap the body in `try { ... } finally { ... }`. Also: since Doris now defaults to Nereids, explicitly setting `enable_nereids_planner = false` at the end leaves the session in a non-default state. Either drop that line or restore it to its captured value rather than hard-coding `false`. ## 12. [Low] Note the latch-before-record ordering in code ```java stmtContext.setQueryStatsRecorded(); try { ... } catch (Exception e) { LOG.warn("Failed to record query stats", e); } ``` Setting the latch before the work is intentional — it prevents a retry from double-counting if the first attempt aborted partway. That's the right choice, but it's not obvious. A one-line comment ("set the latch first so a partial-failure retry does not double-count") would save future readers the trip. ## 13. [Low] Regression coverage for the known gaps The new regression cases all use the form `SELECT col WHERE col = lit`. Add at least one case each for the gaps called out in suggestions 1 and 3 — even if the assertion is `expected = 0` today — so that when Part 2 lands, the test simply flips the expected count and the gap closes visibly. | Scenario | Expected today | Note | |---|---|---| | `SELECT k1 AS x FROM t WHERE k2 = 1` | `k1` queryHit silently dropped | should flip to 1 in Part 2 | | `SELECT COUNT(*) FROM t WHERE k1 = 1` | `k1` filterHit = 1 | already works; locks the behavior | | `SELECT * FROM t1 JOIN t2 ON t1.k = t2.k WHERE t1.x = 1` | join keys dropped, `t1.x` filterHit = 1 | should flip when join conds are mined | | Self-join on the same table | one delta, table count = 1 | locks the dedup contract | --- ## Summary The PR is **focused, narrow, and mergeable** for Part 1. The two changes I'd land before merge are: - Document the scope of `QueryStatsRecorder` (suggestion 6). - Add the four `walkPlan` unit tests (suggestion 2). The largest functional gap is suggestion 1 — `plan.getOutput()` misses more than the PR description implies. If that lands in Part 1, it would meaningfully reduce surprise for users running `SHOW QUERY STATS`. Otherwise the doc change in suggestion 6 is enough to set expectations, and the gap can be closed in Part 2 alongside join-condition coverage. -- 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]
