englefly opened a new pull request, #68990:
URL: https://github.com/apache/doris/pull/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
--
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]
