This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new ddc988bc458 branch-4.2: [fix](fe) Keep deleted rows out of the
simple-agg MIN/MAX cache #68990 (#69179)
ddc988bc458 is described below
commit ddc988bc45847572c4721d6758017415db34e7a8
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Tue Oct 20 15:09:05 2026 +0800
branch-4.2: [fix](fe) Keep deleted rows out of the simple-agg MIN/MAX cache
#68990 (#69179)
Cherry-picked from #68990
Co-authored-by: minghong <[email protected]>
---
.../doris/nereids/stats/SimpleAggCacheMgr.java | 78 +++++++++++++---
.../min_max_after_delete.out | 7 ++
.../min_max_after_delete.groovy | 103 +++++++++++++++++++++
3 files changed, 174 insertions(+), 14 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/stats/SimpleAggCacheMgr.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/stats/SimpleAggCacheMgr.java
index 1b67a7cd35d..7fe378992cb 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/stats/SimpleAggCacheMgr.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/stats/SimpleAggCacheMgr.java
@@ -17,13 +17,18 @@
package org.apache.doris.nereids.stats;
+import org.apache.doris.analysis.UserIdentity;
import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.Env;
import org.apache.doris.catalog.OlapTable;
import org.apache.doris.catalog.TableIf;
+import org.apache.doris.cloud.qe.ComputeGroupException;
import org.apache.doris.common.Config;
+import org.apache.doris.common.FeConstants;
import org.apache.doris.common.ThreadPoolManager;
import org.apache.doris.qe.AutoCloseConnectContext;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.SessionVariable;
import org.apache.doris.qe.StmtExecutor;
import org.apache.doris.statistics.ResultRow;
import org.apache.doris.statistics.util.StatisticsUtil;
@@ -404,6 +409,63 @@ public class SimpleAggCacheMgr {
return "SELECT count(*) FROM " + fullTable;
}
+ /**
+ * Builds the session that the internal SQL of this cache runs on.
+ *
+ * <p>Deliberately not {@code StatisticsUtil.buildConnectContext()}: that
one is shaped for
+ * statistics collection, which only needs an approximate MIN/MAX and
therefore turns on
+ * {@code force_pushdown_zonemap_minmax}. With that on, the BE answers a
pushed-down MIN/MAX from
+ * the segment zone maps even when a delete predicate removed rows, and a
zone map still covers
+ * what that predicate filters out. The cache would then store a deleted
row's bound as the exact
+ * value and hand it to {@code RewriteSimpleAggToConstantRule}.
+ *
+ * <p>So this builder starts from a default {@link SessionVariable} and
sets only what the query
+ * itself needs. Nothing here may trade accuracy for speed.
+ */
+ private static AutoCloseConnectContext buildConnectContext() {
+ ConnectContext connectContext = new ConnectContext();
+ connectContext.getState().setInternal(true);
+ connectContext.setEnv(Env.getCurrentEnv());
+ connectContext.setCurrentUserIdentity(UserIdentity.ADMIN);
+ connectContext.setStartTime();
+ // The internal SQL names its table fully qualified; this only gives
the context a database.
+ connectContext.setDatabase(FeConstants.INTERNAL_DB_NAME);
+ SessionVariable sessionVariable = connectContext.getSessionVariable();
+ // A load is one aggregate over one table, so a single pipeline task
is enough.
+ sessionVariable.parallelPipelineTaskNum = 1;
+ // Disable our own rule to prevent infinite recursion:
+ // this internal SQL goes through Nereids and would otherwise trigger
+ // RewriteSimpleAggToConstantRule again.
+
sessionVariable.setDisableNereidsRules("REWRITE_SIMPLE_AGG_TO_CONSTANT");
+ // Statistics collection turns this on to accept approximate bounds.
This cache promises
+ // exact ones, so a delete predicate has to keep MIN/MAX off the zone
maps. COUNT keeps the
+ // BE's delete-predicate guard whether the switch is on or off.
+ sessionVariable.forcePushDownZonemapMinMax = false;
+ // The cache exists to answer from the table's current visible
version. A result cache keyed
+ // on a session-variable signature must neither answer this query nor
be filled by it.
+ sessionVariable.enableSqlCache = false;
+ sessionVariable.enableQueryCache = false;
+ // Read the base table: a materialized view answers from its own
snapshot.
+ sessionVariable.enableMaterializedViewRewrite = false;
+ // A background scan of a whole column must not evict the pages user
queries are reading, and
+ // this query's result outlives a user query by far once it is cached
in FE, so it reads
+ // storage directly instead of through a block cache. disableFileCache
only takes effect in
+ // cloud mode; elsewhere the block file cache is off regardless.
+ sessionVariable.enablePageCache = false;
+ sessionVariable.disableFileCache = true;
+ if (Config.isCloudMode()) {
+ try {
+ connectContext.getCloudCluster();
+ } catch (ComputeGroupException e) {
+ // Best effort, the way statistics collection treats it: the
planner resolves the
+ // compute group again if this fails, and a load that cannot
run costs the rewrite
+ // rather than correctness.
+ LOG.warn("failed to resolve the compute group of an internal
min/max query", e);
+ }
+ }
+ return new AutoCloseConnectContext(connectContext);
+ }
+
/**
* Async cache loader that issues internal SQL queries to compute exact
min/max.
*/
@@ -454,13 +516,7 @@ public class SimpleAggCacheMgr {
String sql = genMinMaxSql(qualifiers, column.getName());
List<ResultRow> rows;
- try (AutoCloseConnectContext r =
StatisticsUtil.buildConnectContext(false)) {
- r.connectContext.getSessionVariable().setPipelineTaskNum("1");
- // Disable our own rule to prevent infinite recursion:
- // this internal SQL goes through Nereids and would otherwise
trigger
- // RewriteSimpleAggToConstantRule again.
- r.connectContext.getSessionVariable().setDisableNereidsRules(
- "REWRITE_SIMPLE_AGG_TO_CONSTANT");
+ try (AutoCloseConnectContext r = buildConnectContext()) {
StmtExecutor stmtExecutor = new StmtExecutor(r.connectContext,
sql);
rows = stmtExecutor.executeInternalQuery();
}
@@ -523,13 +579,7 @@ public class SimpleAggCacheMgr {
String sql = genCountSql(qualifiers);
List<ResultRow> rows;
- try (AutoCloseConnectContext r =
StatisticsUtil.buildConnectContext(false)) {
- r.connectContext.getSessionVariable().setPipelineTaskNum("1");
- // Disable our own rule to prevent infinite recursion:
- // this internal SQL goes through Nereids and would otherwise
trigger
- // RewriteSimpleAggToConstantRule again.
- r.connectContext.getSessionVariable().setDisableNereidsRules(
- "REWRITE_SIMPLE_AGG_TO_CONSTANT");
+ try (AutoCloseConnectContext r = buildConnectContext()) {
StmtExecutor stmtExecutor = new StmtExecutor(r.connectContext,
sql);
rows = stmtExecutor.executeInternalQuery();
}
diff --git
a/regression-test/data/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.out
b/regression-test/data/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.out
new file mode 100644
index 00000000000..f857b0d4607
--- /dev/null
+++
b/regression-test/data/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.out
@@ -0,0 +1,7 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !min_max_after_delete --
+2022-04-27T16:00:33 2022-04-27T16:00:58 1 2
+
+-- !min_max_after_delete_rule_off --
+2022-04-27T16:00:33 2022-04-27T16:00:58 1 2
+
diff --git
a/regression-test/suites/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.groovy
b/regression-test/suites/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.groovy
new file mode 100644
index 00000000000..035f2cb34ed
--- /dev/null
+++
b/regression-test/suites/nereids_rules_p0/rewrite_simple_agg_to_constant/min_max_after_delete.groovy
@@ -0,0 +1,103 @@
+// 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.
+
+// A no-filter MIN/MAX is rewritten to a constant from SimpleAggCacheMgr. That
cache runs its
+// internal SQL on the statistics connect context, which forces MIN/MAX onto
the segment zone maps.
+// A zone map still covers the rows a delete predicate filters out, so the
cache used to store the
+// max of a row that had been DELETEd and the rewrite handed that value back
to the user.
+suite("min_max_after_delete") {
+ // The rewrite is only reached when the plan is built, so the SQL result
cache would answer the
+ // later queries without ever consulting SimpleAggCacheMgr.
+ sql "SET enable_sql_cache=false"
+
+ sql "DROP DATABASE IF EXISTS test_min_max_after_delete"
+ sql "CREATE DATABASE test_min_max_after_delete"
+ sql "USE test_min_max_after_delete"
+
+ // disable_auto_compaction is what preserves the fault this test is about.
The cached bound is
+ // only wrong while the deleted row is still on disk behind its delete
predicate, so that the
+ // segment zone map still holds it. Auto compaction is on by default, and
a DELETE alone is
+ // enough to make cumulative compaction merge the preceding rowsets and
base compaction apply
+ // the predicate. If that wins the race against the async cache load, the
unfixed code answers
+ // correctly too and this test would pass without covering anything.
+ sql """
+ CREATE TABLE dup_min_max_deleted (
+ k VARCHAR(20) NULL,
+ d DATETIME NULL,
+ n BIGINT NULL
+ ) DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES(
+ 'replication_num' = '1',
+ 'disable_auto_compaction' = 'true'
+ )
+ """
+
+ sql """INSERT INTO dup_min_max_deleted VALUES ('keep_a', '2022-04-27
16:00:33', 1)"""
+ sql """INSERT INTO dup_min_max_deleted VALUES ('keep_b', '2022-04-27
16:00:58', 2)"""
+ // Holds both the max datetime and the max integer, and is deleted right
below. DELETE leaves it
+ // as a delete predicate, so its rows are still on disk behind that
predicate.
+ sql """INSERT INTO dup_min_max_deleted VALUES ('deleted_row', '2024-01-01
00:00:00', 100)"""
+ sql """DELETE FROM dup_min_max_deleted WHERE k = 'deleted_row'"""
+
+ // ===================================================================
+ // Warm up the SimpleAggCacheMgr async cache.
+ //
+ // The first call to getStats() only triggers an async load. Poll until
+ // explain shows "constant exprs", which proves the cache entry -- the
+ // one that used to hold the deleted row's bound -- is in place.
+ //
+ // The query must push down MIN/MAX alone. Adding count(*) would make it
+ // a MIX push-down, which the BE keeps behind its delete-predicate guard,
+ // and the bug would not be exercised.
+ // ===================================================================
+ sql "SELECT min(d), max(d) FROM dup_min_max_deleted"
+ sql "SELECT min(n), max(n) FROM dup_min_max_deleted"
+
+ def minMaxSql = "SELECT min(d), max(d), min(n), max(n) FROM
dup_min_max_deleted"
+ def cacheReady = false
+ for (int i = 0; i < 30; i++) {
+ def explainResult = sql "EXPLAIN ${minMaxSql}"
+ if (explainResult.toString().contains("constant exprs")) {
+ cacheReady = true
+ break
+ }
+ sleep(1000)
+ }
+ if (!cacheReady) {
+ if (isCloudMode()) {
+ logger.info("SimpleAggCacheMgr cache did not warm up within 30s in
cloud mode, skip")
+ return
+ }
+ assertTrue(false, "SimpleAggCacheMgr cache did not warm up within 30
seconds")
+ }
+
+ // The deleted row must not contribute to the answer, neither through the
constant rewrite nor
+ // through the rows the BE reads. Both values below come from the two
surviving rows.
+ explain {
+ sql("${minMaxSql}")
+ contains "constant exprs"
+ }
+ order_qt_min_max_after_delete """SELECT min(d), max(d), min(n), max(n)
FROM dup_min_max_deleted"""
+
+ // A delete predicate has to keep the pushed-down MIN/MAX off the zone
maps, so disabling the
+ // rewrite must agree with the rewritten answer above.
+ order_qt_min_max_after_delete_rule_off """
+ SELECT /*+
SET_VAR(disable_nereids_rules=REWRITE_SIMPLE_AGG_TO_CONSTANT) */
+ min(d), max(d), min(n), max(n) FROM dup_min_max_deleted
+ """
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
