morrySnow commented on code in PR #64892:
URL: https://github.com/apache/doris/pull/64892#discussion_r3765927149
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/AggregateUtils.java:
##########
@@ -145,6 +148,33 @@ public static boolean
hasUnknownStatistics(Collection<Expression> expressions,
return false;
}
+ /**
+ * Whether collected hot-value or null statistics prove that the combined
shuffle keys are skewed.
+ * Missing hot-value statistics mean "no known skew", rather than
disabling an NDV-based optimization.
+ */
+ public static boolean hasKnownSkewOnShuffleKeys(List<Expression>
shuffleKeys, Statistics inputStatistics) {
+ for (int i = 0; i < shuffleKeys.size(); i++) {
+ ColumnStatistic columnStatistic =
inputStatistics.findColumnStatistics(shuffleKeys.get(i));
+ if (columnStatistic == null || columnStatistic.isUnKnown) {
+ continue;
+ }
+ boolean hasHotValue =
StatisticsUtil.getHotValuesWithOriginalThreshold(
+ columnStatistic.getHotValues(), columnStatistic.ndv) !=
null;
+ double nullRatio = columnStatistic.numNulls /
inputStatistics.getRowCount();
+ boolean hasHotNull = columnStatistic.numNulls > 0
Review Comment:
`isHotValueWithOriginalThreshold(nullRatio, ndv)` applies the `ratio * ndv
>= skewValueThreshold` clause to the null ratio. For nulls this clause fires at
extremely small null fractions: since the caller only reaches this check when
`combinedNdv > LOW_NDV_THRESHOLD` holds, any key with null ratio >= 10/ndv is
flagged hot (e.g. ~0.5% nulls at ndv=2000, or 10 nulls per 1M rows at ndv=1M).
Unlike `hotValues` (which the BE only collects when significant values exist),
`numNulls` is always present in column stats, so this makes the rejection the
common case rather than the exception. And when the parent key is a single
column, `otherShuffleKeys` is empty, `estimateGroupByRowCount` returns 1 <=
LOW_NDV_THRESHOLD, so the key is always treated as skewed. The actual
concentration of the null bucket is exactly `nullRatio`, not `nullRatio * ndv`
- note that the existing null convention in `hasSignificantHotValues`
(StatisticsUtil.java:1086) uses only the plain ratio. As written, `aggShuffleU
seParentKey` (default on) will be effectively disabled for any single-column
parent hash key that contains nulls, forcing an extra shuffle. This is a
performance regression rather than a correctness issue, but it is likely much
broader than the OOM scenario described in the PR. Consider restricting the
null check to the ratio clause or adding a minimum null-ratio floor.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -180,37 +175,7 @@ private boolean skewOnShuffleExpr(PhysicalHashAggregate<?
extends Plan> agg) {
return true;
}
// 2. There is a hot value, and the ndv of other keys is very low
- return isSkew(agg.getGroupByExpressions(), inputStatistics);
- }
-
- // if one group by key has hot value, and others ndv is low -> skew
- private boolean isSkew(List<Expression> groupBy, Statistics
inputStatistics) {
- for (int i = 0; i < groupBy.size(); ++i) {
- Expression expr = groupBy.get(i);
- ColumnStatistic colStat =
inputStatistics.findColumnStatistics(expr);
- if (colStat == null || colStat.isUnKnown) {
- continue;
- }
- if
(StatisticsUtil.getHotValuesWithOriginalThreshold(colStat.getHotValues(),
colStat.ndv) == null) {
- continue;
- }
- List<Expression> otherExpr = excludeElement(groupBy, i);
- double otherNdv =
StatsCalculator.estimateGroupByRowCount(otherExpr, inputStatistics);
- if (otherNdv <= AggregateUtils.LOW_NDV_THRESHOLD) {
- return true;
- }
- }
- return false;
- }
-
- private static <T> List<T> excludeElement(List<T> list, int index) {
- List<T> newList = new ArrayList<>();
- for (int i = 0; i < list.size(); i++) {
- if (index != i) {
- newList.add(list.get(i));
- }
- }
- return newList;
+ return
AggregateUtils.hasKnownSkewOnShuffleKeys(agg.getGroupByExpressions(),
inputStatistics);
Review Comment:
This call site changes behavior beyond the parent-key reuse gating described
in the PR description. The old `isSkew` here only considered collected hot
values; the new `hasKnownSkewOnShuffleKeys` additionally treats a *hot-null*
key as skew evidence. For a one-phase agg over a CTE consumer whose group-by
keys all have `hotValues` collected (the `hasUnknownStatistics(..., true)` gate
at line 168 passes only then), a key with >=10% nulls (or nulls >= 10x the
average value frequency) plus other keys' NDV <= LOW_NDV_THRESHOLD will now ban
the one-phase agg. If the null-based skew detection is only intended for the
`shouldUseParent` decision in RequestPropertyDeriver, sharing the helper makes
it leak into this path. Please confirm this is intended (and worth documenting
in the PR description), or keep the null check in the deriver path only.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/util/AggregateUtilsTest.java:
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.util;
+
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
+import org.apache.doris.nereids.types.IntegerType;
+import org.apache.doris.qe.SessionVariable;
+import org.apache.doris.statistics.ColumnStatisticBuilder;
+import org.apache.doris.statistics.Statistics;
+import org.apache.doris.statistics.StatisticsBuilder;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+class AggregateUtilsTest {
+
+ @Test
+ void testKnownSkewOnSingleHotValueKey() {
+ SlotReference key = new SlotReference("key", IntegerType.INSTANCE);
+ Statistics statistics = new StatisticsBuilder()
+ .setRowCount(10000)
+ .putColumnStatistics(key, new ColumnStatisticBuilder(10000)
+ .setNdv(2000)
+ .setHotValues(ImmutableMap.of(Literal.of(1), 0.2f))
+ .build())
+ .build();
+
+ Assertions.assertTrue(AggregateUtils.hasKnownSkewOnShuffleKeys(
+ ImmutableList.of(key), statistics));
+ }
+
+ @Test
+ void testKnownSkewOnSingleNullKey() {
+ SlotReference key = new SlotReference("key", IntegerType.INSTANCE);
+ Statistics statistics = new StatisticsBuilder()
+ .setRowCount(10000)
+ .putColumnStatistics(key, new ColumnStatisticBuilder(10000)
+ .setNdv(2000)
+ .setNumNulls(5000)
+ .setHotValues(ImmutableMap.of())
+ .build())
+ .build();
+
+ Assertions.assertTrue(AggregateUtils.hasKnownSkewOnShuffleKeys(
+ ImmutableList.of(key), statistics));
+ }
+
+ @Test
+ void testKnownSkewDilutedByAnotherHighNdvKey() {
+ SlotReference hotKey = new SlotReference("hot_key",
IntegerType.INSTANCE);
+ SlotReference highNdvKey = new SlotReference("high_ndv_key",
IntegerType.INSTANCE);
+ Statistics statistics = new StatisticsBuilder()
+ .setRowCount(10000)
+ .putColumnStatistics(hotKey, new ColumnStatisticBuilder(10000)
+ .setNdv(2000)
+ .setNumNulls(5000)
+ .setHotValues(ImmutableMap.of())
+ .build())
+ .putColumnStatistics(highNdvKey, new
ColumnStatisticBuilder(10000)
+ .setNdv(2000)
+ .setHotValues(ImmutableMap.of())
+ .build())
+ .build();
+
+ Assertions.assertFalse(AggregateUtils.hasKnownSkewOnShuffleKeys(
+ ImmutableList.of(hotKey, highNdvKey), statistics));
+ }
+
+ @Test
+ void testMissingHotValueStatsAndNoNullAreNotKnownSkew() {
+ SlotReference key = new SlotReference("key", IntegerType.INSTANCE);
+ Statistics statistics = new StatisticsBuilder()
+ .setRowCount(10000)
+ .putColumnStatistics(key, new ColumnStatisticBuilder(10000)
+ .setNdv(2000)
+ .build())
+ .build();
+
+ try (MockedStatic<SessionVariable> mockedSessionVariable =
Mockito.mockStatic(SessionVariable.class)) {
Review Comment:
The mocked thresholds (0.0) do not affect the outcome of this test: the
column has no nulls and no hot values, so `hasKnownSkewOnShuffleKeys` returns
false regardless of the threshold values. The test passes trivially and does
not exercise the 'hot null below/above threshold' boundary implied by the name
and the mock. Consider adding `numNulls` to the column and asserting the result
flips with a threshold change (e.g. mock skewValueThreshold to a tiny value so
`nullRatio * ndv >= skewValueThreshold` becomes decisive), or drop the mock and
state that the assertion is threshold-independent.
--
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]
