github-actions[bot] commented on code in PR #63974:
URL: https://github.com/apache/doris/pull/63974#discussion_r3585361647
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/query/QueryStatsRecorder.java:
##########
@@ -125,19 +128,42 @@ static Map<String, StatsDelta> collectDeltas(PhysicalPlan
plan) {
: plan.getOutput();
for (NamedExpression ne : rootExprs) {
SlotReference sr = unwrapAlias(ne);
- if (sr == null) {
- continue;
+ if (sr != null) {
+ PhysicalOlapScan sourceScan = exprIdToScan.get(sr.getExprId());
+ if (sourceScan != null) {
+ StatsDelta delta = getOrCreateDelta(deltas, sourceScan);
+ if (delta != null) {
+ String colName = sr.getOriginalColumn().map(col ->
col.getName())
+ .orElseGet(() ->
exprIdToColName.get(sr.getExprId()));
+ if (colName != null) {
+ delta.addQueryStats(colName);
+ }
+ }
+ continue;
+ }
}
- PhysicalOlapScan sourceScan = exprIdToScan.get(sr.getExprId());
- if (sourceScan == null) {
+ // Slot from a computed alias, or a complex root alias
(Alias(a+b)): expand via map.
+ ExprId lookupId = (sr != null) ? sr.getExprId() : ne.getExprId();
Review Comment:
This root-output path misses single-input computed SELECT expressions. For
`SELECT k1 + 1 FROM t`, the `PhysicalProject` branch below records the alias
ExprId directly in `exprIdToScan` because the expression has one input slot,
but `unwrapAlias(ne)` is null here and the fallback only checks
`aggOutputToInputSlots`. Since single-input aliases are not put in that map,
the final SELECT output records no `queryHit` for `k1`. Please also resolve
`ne.getExprId()` through the direct scan mapping before falling back to the
multi-input expansion map, and add a case such as `SELECT k1 + 1`.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/query/QueryStatsRecorder.java:
##########
@@ -351,6 +443,36 @@ private static void walkPlan(Plan plan,
exprIdToColName.put(ne.getExprId(), colName);
}
}
+ } else if (inputSlots.size() > 1) {
+ // Defer to avoid misclassifying
PushDownExpressionsInHashCondition
+ // join-key projects as queryHit; parent filter/join
expands as filterHit,
+ // root output loop expands as queryHit.
+ aggOutputToInputSlots.put(ne.getExprId(), inputSlots);
+ }
+ }
+ }
+ }
+ }
+
+ /** Shared by PhysicalSetOperation and PhysicalRecursiveUnion — both
expose the same
+ * getRegularChildrenOutputs() contract and need identical queryHit
recording logic. */
+ private static void recordSetOpChildrenOutputs(
+ List<List<SlotReference>> childrenOutputs,
+ Map<ExprId, PhysicalOlapScan> exprIdToScan,
+ Map<ExprId, String> exprIdToColName,
+ Map<String, StatsDelta> deltas) {
+ for (List<SlotReference> childOutput : childrenOutputs) {
+ for (SlotReference sr : childOutput) {
+ PhysicalOlapScan sourceScan = exprIdToScan.get(sr.getExprId());
Review Comment:
`regularChildrenOutputs` can contain a child project output slot, not just a
scan slot. For a real plan such as `SELECT k1 + k2 FROM t UNION ALL SELECT k1 +
k2 FROM t`, `bindSetOperation` can put each branch's project output into
`regularChildrenOutputs`; the child `PhysicalProject` records that alias only
in the computed-provenance map, while this helper checks only `exprIdToScan`
and skips it. The result is that the set operation records no `queryHit` for
`k1`/`k2`. Please expand through the same computed-alias provenance here (and
for `PhysicalRecursiveUnion`) before deciding the child output has no scan
column.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/query/QueryStatsRecorder.java:
##########
@@ -308,14 +373,42 @@ private static void walkPlan(Plan plan,
}
}
}
- // filterHit for JOIN ON conditions (hash equality and non-equality
predicates).
+ // filterHit for all JOIN ON conditions; mark conjuncts are a separate
field not included
+ // in hashJoinConjuncts or otherJoinConjuncts (IN/EXISTS subquery
semi-join predicates).
if (plan instanceof AbstractPhysicalJoin) {
AbstractPhysicalJoin<?, ?> join = (AbstractPhysicalJoin<?, ?>)
plan;
for (Expression conjunct : join.getHashJoinConjuncts()) {
- recordInputSlotsAsFilterHit(conjunct, exprIdToScan,
exprIdToColName, deltas);
+ recordInputSlotsAsFilterHit(conjunct, exprIdToScan,
exprIdToColName, deltas, aggOutputToInputSlots);
}
for (Expression conjunct : join.getOtherJoinConjuncts()) {
- recordInputSlotsAsFilterHit(conjunct, exprIdToScan,
exprIdToColName, deltas);
+ recordInputSlotsAsFilterHit(conjunct, exprIdToScan,
exprIdToColName, deltas, aggOutputToInputSlots);
+ }
+ for (Expression conjunct : join.getMarkJoinConjuncts()) {
+ recordInputSlotsAsFilterHit(conjunct, exprIdToScan,
exprIdToColName, deltas, aggOutputToInputSlots);
+ }
+ }
+ // UNION / INTERSECT / EXCEPT: record queryHit for each child's
contributing columns.
+ if (plan instanceof PhysicalSetOperation) {
Review Comment:
Recording the branch outputs here is not enough for filters that must stay
above the set operation. `PushDownFilterThroughSetOperation` keeps volatile
predicates above `UNION DISTINCT`/`INTERSECT`/`EXCEPT`, so a plan like
`Filter(k1_set + random() > 0) -> PhysicalSetOperation(...)` can still reach
this recorder. The filter references the set operation's output ExprId, but
this block never maps that output back to the corresponding regular child
output slots, so `recordInputSlotsAsFilterHit` cannot attribute the predicate
to the underlying scan columns. Please also store set-output provenance for
parent filters, not only queryHit for branch outputs.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/query/QueryStatsRecorder.java:
##########
@@ -384,22 +506,47 @@ private static void recordInputSlotsAsQueryHit(Expression
expr,
private static void recordInputSlotsAsFilterHit(Expression expr,
Map<ExprId, PhysicalOlapScan> exprIdToScan,
Map<ExprId, String> exprIdToColName,
- Map<String, StatsDelta> deltas) {
+ Map<String, StatsDelta> deltas,
+ Map<ExprId, Set<Slot>> aggOutputToInputSlots) {
for (Slot slot : expr.getInputSlots()) {
if (!(slot instanceof SlotReference)) {
continue;
}
SlotReference sr = (SlotReference) slot;
PhysicalOlapScan sourceScan = exprIdToScan.get(sr.getExprId());
- if (sourceScan == null) {
- continue;
- }
- StatsDelta delta = getOrCreateDelta(deltas, sourceScan);
- if (delta != null) {
- String colName = sr.getOriginalColumn().map(col ->
col.getName())
- .orElseGet(() -> exprIdToColName.get(sr.getExprId()));
- if (colName != null) {
- delta.addFilterStats(colName);
+ if (sourceScan != null) {
+ StatsDelta delta = getOrCreateDelta(deltas, sourceScan);
+ if (delta != null) {
+ String colName = sr.getOriginalColumn().map(col ->
col.getName())
+ .orElseGet(() ->
exprIdToColName.get(sr.getExprId()));
+ if (colName != null) {
+ delta.addFilterStats(colName);
+ }
+ }
+ } else {
+ // Slot not from a scan — check if it is a multi-input
aggregate output
+ // (e.g. SUM(k2+k3)) and expand to its contributing input
slots.
+ Set<Slot> inputSlots =
aggOutputToInputSlots.get(sr.getExprId());
Review Comment:
This expansion only handles one level of computed provenance. If the
expanded input is itself another computed alias, or if a query-hit context
consumes a computed alias, the base scan columns are still skipped. For
example, `SELECT SUM(x) FROM (SELECT k1 + k2 AS x FROM t) s HAVING SUM(x) > 0`
gives the aggregate `SUM(x)` a single input slot `x`; `x` is only in
`aggOutputToInputSlots`, so `recordInputSlotsAsQueryHit` skips it and the
HAVING setup never maps the aggregate output back to `k1`/`k2`. A recursive
resolver from any slot to base scan-backed slots, reused by both queryHit and
filterHit paths, would cover this and nested join-key aliases.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/query/QueryStatsRecorder.java:
##########
@@ -216,22 +239,43 @@ private static void walkPlan(Plan plan,
}
return;
}
- // TODO: PhysicalCTEConsumer slots use consumer-side ExprIds that
differ from the producer
- // scan's ExprIds, so CTE column stats are silently missed. Fix
requires mapping consumer
- // slots back to producer slots via
StatementContext.getConsumerToProducerSlotMap().
+ // PhysicalCTEProducer: walk its single child so the producer scan is
registered
+ // before any PhysicalCTEConsumer nodes are encountered during the
tree walk.
+ // Uses plan.children() for consistency with the rest of walkPlan.
+ if (plan instanceof PhysicalCTEProducer) {
+ walkPlan(plan.children().get(0),
+ exprIdToScan, exprIdToColName, deltas,
aggOutputToInputSlots);
+ return;
+ }
+ // PhysicalCTEConsumer: map consumer slots to producer scan slots so
parent
+ // plan nodes can resolve CTE column references correctly.
+ // Mapping is done before the children() loop so it is populated
regardless of
+ // whether the consumer has children — consistent with how scan
handlers work.
+ if (plan instanceof PhysicalCTEConsumer) {
+ PhysicalCTEConsumer cteConsumer = (PhysicalCTEConsumer) plan;
+ for (Slot consumerSlot : cteConsumer.getOutput()) {
+ Slot producerSlot = cteConsumer.getProducerSlot(consumerSlot);
+ PhysicalOlapScan sourceScan =
exprIdToScan.get(producerSlot.getExprId());
Review Comment:
This only copies CTE consumer slots when the producer slot already has a
direct `exprIdToScan` entry. For a materialized CTE that produces a computed
column, for example `WITH cte AS (SELECT k1 + k2 AS x FROM t) SELECT x FROM
cte`, the producer-side `PhysicalProject` stores `x` only in
`aggOutputToInputSlots` because it has multiple input slots. There is no direct
scan mapping for `x`, so this branch skips the consumer slot and the root
output or a parent filter on `x` never records `queryHit`/`filterHit` for `k1`
or `k2`. Please carry the computed provenance across the CTE boundary as well,
preferably through a shared resolver that expands a slot to its base scan slots.
--
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]
