yujun777 commented on code in PR #69896:
URL: https://github.com/apache/doris/pull/69896#discussion_r4462607078
##########
be/src/storage/task/engine_publish_version_task.cpp:
##########
@@ -248,6 +250,18 @@ Status EnginePublishVersionTask::execute() {
tablet_id_to_num_delta_rows.insert(
{rowset_meta_ptr->tablet_id(),
rowset_meta_ptr->num_rows()});
}
+ // A load which writes a changelog keeps its changelog rowset on
the base tablet's
+ // transaction info instead of committing a changelog tablet of
its own, so its rows are
+ // reported separately below, once the publish tasks have
finished: a merge-on-write
+ // publication can still append changelog segments to that rowset.
The frontend cannot
+ // derive them from the rows it loaded into the base tablets -- an
update writes change
+ // rows without adding a base row, and a delete adds none to the
base but at least one
+ // to the changelog -- and it keeps the count to estimate how many
rows a read of the
+ // changelog returns.
Review Comment:
Updated in 9fb10c4c0e1. The FE now applies changelog deltas directly to the
partition's MaterializedIndex row-count estimate; the TableStatsMeta lifetime
accumulator is gone. A later tablet-stat report refreshes the physical count,
rather than repairing a cumulative counter. My earlier reply conflated those
two mechanisms.
Cross-attempt publish reporting can still omit a tablet that succeeded in an
earlier failed attempt, just as for base deltas. This remains a documented
limitation; preserving deltas across attempts should cover both paths in a
follow-up.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1525,6 +1535,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event)
{
+ "rows for all replica {}, tablets count {}",
olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
Review Comment:
The cloud limitation remains explicit in the updated description. Cloud
commits do not provide per-index changelog deltas, so this PR only updates
shared-nothing publish reporting. Cloud changelog counts still come from
asynchronous tablet statistics.
In 9fb10c4c0e1 the lifetime accumulator and its zero-valued fallback were
removed; local deltas now update the partition index read by P/Q. My earlier
description of the cloud fallback as safe was too strong: stale changelog/base
counts do not guarantee an upper bound. Extending the cloud commit response
remains a separate change.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -391,6 +397,12 @@ public long getBaseIndexDeltaRowCount(OlapTable table) {
* {@link #updatedRows} and reports them as delta rows.
*/
public synchronized long getRowCountWithDeltaRows(OlapTable table, long
indexId) {
+ if (isChangelogIndex(table, indexId)) {
+ // The changelog index has no collected row count: the rows a load
wrote to it are the
+ // only count this record keeps, and they are added rather than
taken as a lower bound
Review Comment:
Fixed in 9fb10c4c0e1 by removing the TableStatsMeta changelog accumulator
and its row-count fallback. Published deltas now update the partition's
changelog MaterializedIndex directly, preserving its existing row-count
estimate independently of DROP STATS or statistics-record recreation. An
unavailable index count starts from the reported load delta as a best-effort
estimate; it is not claimed to contain unknown historical rows.
The new AnalysisManagerTest case verifies that removing/recreating
TableStatsMeta leaves the public changelog row-count lookup unchanged.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -414,6 +426,31 @@ public synchronized long
getRowCountWithDeltaRows(OlapTable table, long indexId)
return rowCount + getBaseIndexDeltaRowCount(table);
}
+ /**
+ * Add the rows a load wrote to an index which has no collected row count
of its own.
+ */
+ public synchronized void addIndexUpdatedRows(long indexId, long rows) {
+ if (rows <= 0) {
+ return;
+ }
+ indexUpdatedRows().merge(indexId, rows, Long::sum);
+ }
Review Comment:
Fixed in 9fb10c4c0e1. Changelog load deltas now update each partition's own
MaterializedIndex, rather than an index-wide lifetime accumulator in
TableStatsMeta. Partial TRUNCATE replaces the affected partition/index and
therefore discards its old estimate while retaining the other partitions'
counts.
The new test replaces one partition, verifies that the old rows disappear
from the public row-count lookups, and checks that late reports for the old
partition do not repopulate the replacement.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1512,8 +1513,17 @@ public void replayUpdateRowsRecord(UpdateRowsEvent
event) {
LOG.info("Tablet row count map is empty");
continue;
}
+ long changelogIndexId = olapTable.hasChangelog()
+ ? olapTable.getChangelogIndexMeta().getIndexId() : -1;
long rowsForAllReplica = 0;
+ long changelogRowsForAllReplica = 0;
for (Entry<Long, Long> entry : tabletRows.entrySet()) {
+ if (changelogIndexId > 0 &&
isChangelogTablet(entry.getKey(), changelogIndexId)) {
+ // The change rows of the load, which are neither the
rows loaded into the
+ // base index nor a lower bound of the table's rows:
see TableStatsMeta.
Review Comment:
Fixed in 9fb10c4c0e1. For changelog tables, tablet metadata and the owning
partition are resolved before classifying any reported tablet. Missing metadata
or a removed partition causes that entry to be skipped, rather than falling
through into base updatedRows.
AnalysisManagerTest covers both deleted tablet metadata and a replaced
partition whose old tablet metadata is still present, and verifies that neither
case inflates the base counter.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -391,6 +397,12 @@ public long getBaseIndexDeltaRowCount(OlapTable table) {
* {@link #updatedRows} and reports them as delta rows.
*/
public synchronized long getRowCountWithDeltaRows(OlapTable table, long
indexId) {
+ if (isChangelogIndex(table, indexId)) {
+ // The changelog index has no collected row count: the rows a load
wrote to it are the
+ // only count this record keeps, and they are added rather than
taken as a lower bound
+ // of the table's rows.
Review Comment:
The wrapper lookup issue belongs to the consumer change in #69850, which
adds row-count delegation and stream-specific estimates. It is not fixed by
this PR alone.
In 9fb10c4c0e1 this PR no longer introduces a TableStatsMeta fallback:
published deltas update the partition changelog MaterializedIndex directly, so
the existing partition-index lookup used by P/Q reads the updated estimate. The
new test exercises both public partition and whole-index row-count lookups. The
wrapper work remains scoped to #69850.
--
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]
