englefly commented on code in PR #68282:
URL: https://github.com/apache/doris/pull/68282#discussion_r4064238175
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3904,7 +3904,7 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
if (truncateEntireTable) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().resetTableStats(olapTable);
Review Comment:
Fixed in 643c00b3ce0 — thank you, this was a real defect of the previous
revision.
`Env.replayTruncateTable()` removed the record right after
`InternalCatalog.replayTruncateTable()` had reset it, so the reset only existed
on the DDL path: on a follower, and on any FE restart whose journal range
covers the truncate, the record was dropped again and the fix was undone. The
whole-table branch is now empty there, only a partition truncation is accounted
in `Env` (`if (!info.isEntireTable())`), and the reset is performed by
`InternalCatalog.replayTruncateTable()` for every replay. That also keeps old
journals compatible: the reset does not depend on any field of
`TruncateTableInfo`, so an entry written before this change resets through the
same code path.
Verified on a local FE+BE cluster: `TRUNCATE TABLE t; INSERT 5 rows` ->
`SHOW TABLE STATS` gives `updated_rows=5, columns=[]`; restart the FE with the
truncate in the replayed journal range -> still `updated_rows=5`; loading two
more rows afterwards -> `updated_rows=7`.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -105,6 +105,13 @@ public class TableStatsMeta implements Writable,
GsonPostProcessable {
@SerializedName("irc")
private ConcurrentMap<Long, Long> indexesRowCount = new
ConcurrentHashMap<>();
+ // The value of updatedRows when indexesRowCount was collected, i.e. the
number of rows the collected
+ // row count already includes. The rows loaded after that point are the
delta row count of the table.
+ // It is kept here, and not derived from colToColStatsMeta, so that
dropping the column statistics of
+ // the table doesn't lose it. -1 means no row count has ever been
collected from the table.
+ @SerializedName("updatedRowsBase")
Review Comment:
Fixed in 643c00b3ce0 — this was a regression of the previous revision.
`AnalysisInfo.indexesRowCount` is only filled for the indexes a job actually
collected (`BaseAnalysisTask` calls `addIndexRowCount(indexId,
colStatsData.count)`, with `indexId = info.indexId == -1 ? baseIndexId :
info.indexId`), so the baseline is now advanced only when
`analyzedJob.indexesRowCount` contains the base index id:
```java
if (!analyzedJob.userInject &&
analyzedJob.indexesRowCount.containsKey(olapTable.getBaseIndexId())) {
updatedRowsBase.set(analyzedJob.updateRows);
}
```
An MVP-only or named-partition analysis no longer moves the baseline of the
base index, which restores the previous `max()`-over-base-index-column-stats
semantics. Covered by the new unit test
`testDeltaRowCountBaselineNotAdvancedWithoutBaseIndexRowCount` (a job which
collected index 10002 only leaves the delta at 50).
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1487,6 +1487,38 @@ public void removeTableStats(long tableId) {
}
}
+ /**
+ * TRUNCATE TABLE removes all the data of the table, but the table itself
stays and can be loaded
+ * immediately. The stats record must be kept, otherwise the row count of
the newly loaded data can
+ * never be reported: the backends report the row count of the new empty
tablets with a delay of up to
+ * {@code tablet_stat_update_interval_second}, and without a record there
is nothing to accumulate the
+ * loaded rows into. So reset the record to the state of an empty table
instead of removing it.
+ */
+ public void resetTableStats(OlapTable table) {
+ // Keep the followers and the checkpoint image consistent with the
master.
+ logCreateTableStats(resetTableStatsInternal(table));
Review Comment:
Fixed in 643c00b3ce0.
You are right that the entry is not atomic with the truncate entry.
`resetTableStats()` no longer journals anything: the transition is carried by
the truncate journal entry itself, and both the DDL path
(`InternalCatalog.truncateTable`) and the replay path
(`InternalCatalog.replayTruncateTable`) apply it. A crash can therefore only
leave either "no truncate entry, no reset" or "truncate entry, reset", and the
truncate entry alone is enough to reconstruct the state on followers, restart
and checkpoint recovery. This also removes the extra journal write per truncate.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -130,6 +137,58 @@ public TableStatsMeta(long rowCount, AnalysisInfo
analyzedJob, TableIf table) {
update(analyzedJob, table);
}
+ /**
+ * Create a record for a table which doesn't have one yet, in the state of
an empty table. The rows
+ * loaded into the table are accumulated by {@link
AnalysisManager#replayUpdateRowsRecord}, so a record
+ * has to exist before the first load, otherwise these rows can never be
turned into a row count.
+ */
+ public TableStatsMeta(OlapTable table) {
+ this.ctlId = table.getDatabase().getCatalog().getId();
+ this.ctlName = table.getDatabase().getCatalog().getName();
+ this.dbId = table.getDatabase().getId();
+ this.dbName = table.getDatabase().getFullName();
+ this.tblId = table.getId();
+ this.tblName = table.getName();
+ this.idxId = -1;
+ this.indexesRowCount = buildEmptyIndexRowCount(table);
Review Comment:
Acknowledged, with one correction on the analyze case and one piece of
evidence on the drop-stat case.
**Analyze finishing after the truncate.** The same stale snapshot is applied
today, the record just gets created instead of updated: before this change
`findTableStatsStatus()` returned null for the truncated table, so the stale
job took the `new TableStatsMeta(jobInfo.rowCount, jobInfo, tbl)` branch
carrying the same pre-truncate row count, baseline and column metas. Keeping
the record changes where the snapshot lands, not what it contains. The
arithmetic is also self-correcting, because the stale collected row count and
the stale baseline cancel each other: a stale snapshot with row count 100 and
baseline 100 on a table which has 5 rows after the truncate reports `100 + (5 -
100) = 5`, which is the truth. Covered by
`testDeltaRowCountOfSnapshotTakenBeforeReset`.
**`DropStatsTask`.** Agreed that the captured record is now live rather than
detached, so a task running after a newer `ALTER TABLE ... SET STATS` can clear
the user supplied statistics. Note that this window already exists today for
`TRUNCATE TABLE t PARTITION(p)` (a partition truncation never removed the
record) and for every analyzed table, so it is an upstream robustness gap that
this change widens to whole-table truncations rather than introduces.
Both cases want the same mechanism: a generation/epoch on the record which
every asynchronous producer captures and re-checks, plus, for the load path,
filtering the tablet deltas by the partitions which survived the truncate. I
did not add it in this revision because a fence covering only the drop-stat
task would be a half-measure, and the load-path filter needs a tablet existence
check that has to stay cheap on the load path (`TabletInvertedIndex` is not
usable — truncated tablets stay in it until the recycle bin expires). I'm happy
to do it as a follow-up PR, or here, whichever you prefer.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -3904,7 +3904,7 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
oldPartitions = truncateTableInternal(olapTable, newPartitions,
truncateEntireTable, recyclePartitionParamMap, forceDrop,
version, versionTimeMs);
if (truncateEntireTable) {
-
Env.getCurrentEnv().getAnalysisManager().removeTableStats(olapTable.getId());
+
Env.getCurrentEnv().getAnalysisManager().resetTableStats(olapTable);
Review Comment:
Confirmed in the code, and thank you for tracing it precisely.
`finishCheckPartitionVersion()` removes the partition commit info of a
partition which no longer exists (`DatabaseTransactionMgr` line ~1416), but
`updateCatalogAfterVisible()` forwards
`transactionState.getTableIdToTabletDeltaRows()` (line ~2664) without
filtering, so the rows of tablets discarded by the truncate still reach
`AnalysisManager.updateUpdatedRows()` and are summed per table id in
`replayUpdateRowsRecord()`. Before this change the record was gone at that
point, so the update was silently dropped by the
`idToTblStats.get(record.getKey()) == null` check; keeping the record makes
this pre-existing gap observable, so it is a valid objection against this PR.
Impact is bounded: the inflated value is only used by the planner fallback
while the backends have not reported the new tablets yet, and it is replaced by
the reported count or by the next analysis. `Statistics.deltaRowCount`
consumers ignore it unless it is positive (`FilterEstimation` line ~94), and
`getOlapTableRowCount()` reports `collected row count + delta` with a floor of
1, so a too large delta only over-estimates.
I would prefer to fix it where the information exists — filter the delta map
by the partitions/tablets which are still part of the table when the
transaction becomes visible, or carry the truncate generation in
`TruncateTableInfo` and drop the deltas of a transaction which committed before
it. I did not fold it into this PR because it changes the shared
transaction/load accounting path and needs a per-tablet existence check which
must remain cheap per load. I'll send it as a separate PR if that works for you.
--
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]
