github-actions[bot] commented on code in PR #66898:
URL: https://github.com/apache/doris/pull/66898#discussion_r4043388612
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/StatsCalculator.java:
##########
@@ -620,6 +637,22 @@ public Statistics computeOlapScan(OlapScan olapScan) {
return computeVirtualColumnStats(olapScan, builder.build());
}
+ /**
+ * Returns the slots whose column stats should be fetched for the query.
+ *
+ * <p>Only operative slots' column stats are needed by the query, column
stats of other slots are
+ * useless and fetching them would pollute the column stats cache and
waste time on wide tables.
+ * If operative slots are not derived yet (e.g. stats derivation during
RBO) or full stats
+ * fidelity is required (forbidUnknownColStats), fall back to all output
slots.
+ */
+ private List<Slot> getStatsNeededSlots(OlapScan olapScan) {
+ if (forbidUnknownColStats) {
Review Comment:
[P1] Preserve a derived-empty operative set. The existing test establishes
this real plan shape:
```text
PhysicalResultSink(*)
OlapScan(wide_table, operative=[])
```
`OperativeColumnDerive` legitimately writes the empty list, but this
fallback treats the same value as 'derivation not run' and expands it to every
scan output. The scan therefore calls the loading cache API for every visible
column, defeating the optimization. Please represent derivation state
separately from the slot collection (or otherwise distinguish a derived empty
set) and cover derivation plus scan-stat loading end to end.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/cache/StatisticsCache.java:
##########
@@ -136,6 +155,28 @@ private ColumnStatistic doGetColumnStatistics(
return ColumnStatistic.UNKNOWN;
}
+ /**
+ * Returns the column statistic only when it is already present in the
cache, without
+ * triggering a cache load. Used for columns whose stats are not needed by
the query,
+ * so that useless columns do not pollute the cache nor issue queries to
the stats table.
+ */
+ private ColumnStatistic doGetColumnStatisticsIfPresent(
+ long catalogId, long dbId, long tblId, long idxId, String colName,
ConnectContext ctx) {
+ StatisticsCacheKey k = new StatisticsCacheKey(catalogId, dbId, tblId,
idxId, colName);
+ CompletableFuture<Optional<ColumnStatistic>> f =
columnStatisticsCache.getIfPresent(k);
Review Comment:
[P2] Use a quiet cache probe here. In Caffeine 3.2.3,
[`LocalAsyncCache.getIfPresent`](https://github.com/ben-manes/caffeine/blob/v3.2.3/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java#L64-L67)
delegates to the recording lookup, and
[`BoundedLocalCache.getIfPresent`](https://github.com/ben-manes/caffeine/blob/v3.2.3/caffeine/src/main/java/com/github/benmanes/caffeine/cache/BoundedLocalCache.java#L2044-L2071)
calls `afterRead`, which invokes `refreshIfNeeded`. A refresh-eligible
non-operative entry can therefore call the loader and query the statistics
table, while every hit also updates access policy. That contradicts this
method's no-load/no-pollution contract. Please use the synchronous view's quiet
policy probe
(`columnStatisticsCache.synchronous().policy().getIfPresentQuietly(k)`) and add
a real-cache refresh test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java:
##########
@@ -415,6 +415,13 @@ public class Rewriter extends AbstractBatchJobExecutor {
topDown(new LimitSortToTopN()),
topDown(new SplitLimit()),
custom(RuleType.SET_PREAGG_STATUS,
SetPreAggStatus::new),
+ // Derive operative columns on the plan
recorded for materialized view
+ // pre rewrite: the pre rewrite runs a
cost-based optimization on this
+ // recorded plan to choose the best
materialized view, and without
+ // operative slots computeOlapScan would
fall back to fetching column
+ // stats of all table columns. The
derivation is repeated before "init
+ // join" and at the end of rewrite, where
the operative slots of the
+ // plans finally stored into the memo are
recomputed.
custom(RuleType.OPERATIVE_COLUMN_DERIVE,
OperativeColumnDerive::new),
Review Comment:
[P2] The MV-stage pass is still too late for this reachable CTE shape:
```text
CTEAnchor
CTEProducer -> Project(a) -> Scan(wide[a,b,...], operative=[])
CTEConsumer
```
`RewriteCteChildren.visitLogicalCTEAnchor` first runs `StatsDerive` on
`cteAnchor.child(0)` (lines 93-94) and only later recursively applies this job
list to the producer. The normal whole-tree CTE path has the same prepass, so
it initiates a load for every visible column before either new derivation can
help. Please derive producer operative slots before that prerequisite stats
pass (or defer it), with ordinary-CTE and MV-pre-rewrite coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java:
##########
@@ -678,6 +685,13 @@ public class Rewriter extends AbstractBatchJobExecutor {
topDown(new PushDownAggThroughJoinOnPkFk()),
topDown(new PullUpJoinFromUnionAll())
),
+ // RBO rules that depend on statistics (e.g. InitJoinOrder,
SkewJoin, Eager
+ // aggregation, DecomposeRepeatWithPreAggregation,
DistinctAggStrategySelector)
+ // must be placed AFTER OperativeColumnDerive:
StatsCalculator.computeOlapScan
+ // only fetches column stats of operative slots, so rules
running before the
+ // derivation would fetch stats of all table columns,
polluting the column stats
+ // cache and wasting time on wide tables.
+ custom(RuleType.OPERATIVE_COLUMN_DERIVE,
OperativeColumnDerive::new),
Review Comment:
[P2] Run this before `InferSetOperatorDistinct`. The reachable order is:
```text
Intersect DISTINCT
Project(a)
OlapScan(wide[a,b,...], operative=[])
```
That rule is scheduled at line 614 and calls `StatsDerive` when stats are
absent, so the new fallback initiates loads for every visible column before
this line runs. This is distinct from the existing thread about removing other
passes: the defect is an earlier stats consumer bypassing all three. Please
place operative derivation before the first rewrite-time stats consumer and add
a planner-level set-operation case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java:
##########
@@ -826,6 +841,11 @@ public class Rewriter extends AbstractBatchJobExecutor {
)
),
topDown(new CollectCteConsumerOutput()),
+ // Re-derive operative columns at the end of rewrite: rules
after the early
+ // OperativeColumnDerive (before "init join") may rebuild
scans or add virtual
+ // columns (e.g. stream scan normalization, variant virtual
column push down),
Review Comment:
[P2] Re-derive immediately after stream-scan normalization. The reduced
post-normalization tree is:
```text
Join(stream.k = t.k)
Project -> replacement OlapScan(wide, operative=[])
Scan(t)
```
Normalization creates fresh scans with empty operative lists, and `SkewJoin`
runs before this final pass and calls `StatsDerive` on unstatted children. It
can therefore initiate all-column loads before line 849 repairs the slots. This
is distinct from the existing reply that normalization requires a final pass:
the intervening stats consumer runs before that repair. Please move/add the
pass before the first post-normalization stats consumer and cover a wide
stream-scan join.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/StatsCalculator.java:
##########
@@ -609,7 +619,14 @@ public Statistics computeOlapScan(OlapScan olapScan) {
} else {
// get table level stats
for (SlotReference slot : visibleOutputSlots) {
- ColumnStatistic cache =
olapTableStats.getColumnStatistics(slot.getName(), connectContext);
+ ColumnStatistic cache;
+ if (!statsNeededSlots.contains(slot)) {
Review Comment:
[P1] Keep carried-output width deterministic. A reduced reachable tree is:
```text
ResultSink(r.payload)
Project(r.payload)
HashJoin(l.k = r.k)
Scan(l.k)
Scan(r.k, r.payload)
```
Only `r.k` is operative, but `r.payload` remains in the join output. Its
`avgSizeByte` is consumed by `computeTupleSize`, exchange costs, and
`checkBroadcastJoinStats`' hard memory gate. Here a cold cache gives UNKNOWN
(`avgSizeByte=1`) while an unrelated warm cache gives the analyzed width, so
the same wide build can alternate between broadcast-eligible and ineligible.
Please use a cache-independent type/schema width for unloaded outputs or
include every slot used by byte-size gates, and test cold versus prewarmed
behavior.
--
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]
