This is an automated email from the ASF dual-hosted git repository.
englefly pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 28c9f8d8705 [fix](fe) Keep deleted rows out of the simple-agg MIN/MAX
cache (#68990)
28c9f8d8705 is described below
commit 28c9f8d8705d761b8b70636a69d4d60be2d7c432
Author: minghong <[email protected]>
AuthorDate: Fri Oct 16 14:35:48 2026 +0800
[fix](fe) Keep deleted rows out of the simple-agg MIN/MAX cache (#68990)
### What problem does this PR solve?
Problem Summary:
A no-filter, no-GROUP BY `SELECT min(c), max(c) FROM t` on a
DUPLICATE-key table
is answered by `RewriteSimpleAggToConstantRule` from the
SimpleAggCacheMgr cache.
That cache ran its internal SQL on
`StatisticsUtil.buildConnectContext()`, the
session builder statistics collection uses, which turns on
`force_pushdown_zonemap_minmax` because statistics only needs an
approximate
bound.
With that switch 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. So once a DELETEd row held the column's max,
the
cache stored that deleted value:
CREATE TABLE t (k VARCHAR(20) NULL, v DATETIME NULL) DUPLICATE KEY(k)
DISTRIBUTED BY HASH(k) BUCKETS 1 PROPERTIES ('replication_num' = '1');
INSERT INTO t VALUES ('a', '2022-04-27 16:00:33'), ('b', '2022-04-27
16:00:58');
INSERT INTO t VALUES ('1', '2024-01-01 00:00:00');
DELETE FROM t WHERE k = '1';
SELECT min(v), max(v) FROM t; -- 16:00:33 / 16:00:58: reads the rows and
-- kicks off the background cache load
SELECT min(v), max(v) FROM t; -- 16:00:33 / 2024-01-01 00:00:00 -- wrong
From the second query on the answer is a constant, so it stops depending
on the
table at all, and the deleted value is served until the table's version
changes,
the entry is evicted, or the FE restarts. Base compaction does not clear
it
either: the entry is validated on the table's visible version, which
compaction
does not change. Numeric and date/datetime columns are affected alike; a
string
column is not, because MIN/MAX is not cached for it. COUNT is not
affected:
MIN/MAX is the only aggregate the switch can force, so the BE keeps the
delete-predicate guard for COUNT with the switch on or off.
The cache promises exact values, so it must not inherit a switch that
exists to
accept approximate ones. It now builds its own session and sets only
what its
query needs, so a statistics-only switch cannot reach it again by
default. Both
loaders share that builder, which also drops the session setup they had
duplicated.
Kept deliberately, each for the query rather than for statistics:
- the page cache stays off, and the block file cache is disabled in
cloud mode: a
background scan of a whole column must not evict the pages user queries
are
reading, and its result outlives a user query by far once FE has cached
it;
- the SQL cache and query cache stay off: this cache answers from the
table's
current visible version, so a result cache must neither answer the query
nor be
filled by it;
- materialized view rewrite stays off: read the base table, not a
snapshot.
Dropped, because they are statistics' own tuning rather than the query's
shape:
the statistics memory and CPU budgets, the analyze timeout reused as the
query
and insert timeout, and the statistics parallel instance count.
### Release note
Fixed a wrong `min()`/`max()` result on a DUPLICATE-key table after a
`DELETE`.
A repeated no-filter `SELECT min(c), max(c)` could return a value that
only a
deleted row held, until the table's version changed or the FE was
restarted.
### Check List (For Author)
- Test: Regression test, Unit Test, Manual test
- Added `min_max_after_delete.groovy` to the existing
`nereids_rules_p0/rewrite_simple_agg_to_constant` suite. Its `.out` was
generated on the parent commit, where the rewritten query recorded the
deleted row's bounds (`2024-01-01T00:00`, `100`) while the rule-disabled
query returned the surviving rows' bounds; with this fix both record the
same, correct bounds.
- Ran that whole suite directory: 4 suites, 0 failures.
- Ran `RewriteSimpleAggToConstantRuleTest`: 15 tests, 0 failures.
- Manually confirmed on a cluster that after a DELETE the MIN/MAX
rewrite
still fires with the surviving rows' bound, that the COUNT rewrite still
fires and follows an INSERT, that the result holds across a base
compaction, and that a DELETE after the cache is warm reloads it.
- Behavior changed: Yes. The wrong `min()`/`max()` described above is no
longer
returned; correct results are unchanged and the constant rewrite still
applies.
The internal session of this cache no longer inherits statistics'
resource
budgets, so a load that no longer fits them (a default 900s query
timeout
instead of the 12h analyze timeout) times out and costs the rewrite
rather
than correctness.
- Does this need documentation: No
---
.../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 290c65322a0..8b31247a158 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.repository.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]
