github-actions[bot] commented on code in PR #69028:
URL: https://github.com/apache/doris/pull/69028#discussion_r4266628223
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -194,6 +273,21 @@ public void update(AnalysisInfo analyzedJob, TableIf
tableIf) {
if (tableIf != null) {
if (tableIf instanceof OlapTable) {
OlapTable olapTable = (OlapTable) tableIf;
+ // The collected row counts published below already include
the rows which had been loaded when
+ // the job was built, remember how many they were, they are
not delta rows. The baseline may
+ // only advance together with the collected base index row
count, an analysis of another index
+ // (a materialized view) doesn't touch it.
+ // NOTICE: the baseline is published before the collected
counts. They are both read without
+ // the table lock by the planner, and a reader which pairs the
collected count of this analysis
+ // with the baseline of the previous one counts the rows
loaded since that previous analysis
+ // twice.
+ if
(analyzedJob.indexesRowCount.containsKey(olapTable.getBaseIndexId())) {
+ // A row count supplied by the user replaces the collected
one and describes the table as
+ // of now, so the rows loaded so far are part of it and
the baseline has to move to the
+ // current value. Keeping the older baseline would count
those rows again once DROP STATS
+ // clears userInjected.
+ updatedRowsBase.set(analyzedJob.userInject ?
updatedRows.get() : analyzedJob.updateRows);
Review Comment:
[P2] Align the baseline with the snapshot that ANALYZE actually reads
This baseline is captured when the job is built, while indexesRowCount comes
from SQL executed later. On a fresh or truncated table, load 100 rows, build a
FULL ANALYZE job (updateRows=100), then let another 50-row load become visible
before its COUNT(1) query starts. The new load-created record has
updatedRows=150 and analysis collects 150, but this line installs baseline=100,
so a full OlapScan before tablet reporting gets cardinality 150+(150-100)=200.
Previously the first analysis had no existing record/counter, so this sequence
returned the collected 150. The monitor cannot reconcile these two different
data snapshots. Pair the collected count with the load baseline for the
visible-version snapshot used by the analyze query and add a deterministic
load-between-job-build-and-query case.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/TableStatsMeta.java:
##########
@@ -268,6 +382,29 @@ public long getBaseIndexDeltaRowCount(OlapTable table) {
return updatedRows.get() - maxUpdateRows;
}
+ /**
+ * The row count of the index together with the rows loaded since it was
collected, i.e. the row count of
+ * the table. Both are read by the planner without the table lock, for
instance while it plans a direct
+ * scan of a materialized view, so they have to come from the same state
of this record: a collected row
+ * count paired with the baseline of another analysis would count rows
twice or miss them. The rows loaded
+ * while this call runs are not part of the snapshot, whichever state
reads them accumulates them in
+ * {@link #updatedRows} and reports them as delta rows.
+ */
+ public synchronized long getRowCountWithDeltaRows(OlapTable table, long
indexId) {
+ long rowCount = getRowCount(indexId);
+ if (!keepsOneRowPerBaseRow(table, indexId)) {
+ // The index aggregates, or it merges the rows of a unique key
table: it has its own, smaller row
+ // count, and the rows loaded into the base index would overstate
it.
+ return rowCount;
+ }
+ if (rowCount == TableIf.UNKNOWN_ROW_COUNT) {
+ // The index has no collected row count yet, neither the row count
of the table nor a delta row
+ // count can be derived from it. The planner treats the unknown
row count as no information.
+ return rowCount;
+ }
+ return rowCount + getBaseIndexDeltaRowCount(table);
Review Comment:
[P2] Keep a baseline for an independently injected rollup count
A row-preserving rollup can have a current collected count while
updatedRowsBase still describes the base index's older snapshot. For a fresh
DUP table t(k1,k2,v) with r_dup(k1,v), load 5 rows, run ALTER TABLE t INDEX
r_dup MODIFY COLUMN k1 SET STATS ('row_count'='5'), then DROP STATS t(k2). The
injection updates only r_dup's count, so the base-index condition in update()
leaves the new record's baseline at 0; column DROP STATS keeps that count but
clears userInjected. Before tablet reporting, Project(k1,v) -> OlapScan(t,
index=r_dup) therefore returns 5+(5-0)=10 here although the rollup has 5 rows.
Previously rollup-only injection created a record with no accumulated rows and
returned 5. Pair each index count with its own load baseline, or consistently
refresh equivalent index snapshots when injecting a count, and cover this
injection/drop sequence.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1366,52 +1405,52 @@ public void replayUpdateRowsRecord(UpdateRowsEvent
event) {
// Record : TableId -> (TabletId -> update rows)
if (event.getTabletRecords() != null) {
for (Entry<Long, Map<Long, Long>> record :
event.getTabletRecords().entrySet()) {
- TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
- if (statsStatus != null) {
- Optional<Database> dbOption =
catalog.getDb(event.getDbId());
- if (!dbOption.isPresent()) {
- LOG.warn("Database {} does not exist.",
event.getDbId());
- continue;
- }
- Database database = dbOption.get();
- Optional<Table> tableOption =
database.getTable(record.getKey());
- if (!tableOption.isPresent()) {
- LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
- continue;
- }
- Table table = tableOption.get();
- if (!(table instanceof OlapTable)) {
- continue;
- }
- OlapTable olapTable = (OlapTable) table;
- short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
- Map<Long, Long> tabletRows = record.getValue();
- if (tabletRows == null || tabletRows.isEmpty()) {
- LOG.info("Tablet row count map is empty");
- continue;
- }
- long rowsForAllReplica = 0;
- for (Entry<Long, Long> entry : tabletRows.entrySet()) {
- rowsForAllReplica += entry.getValue();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Table id {}, tablet id {}, row count
{}",
- record.getKey(), entry.getKey(),
entry.getValue());
- }
- }
- long tableUpdateRows = rowsForAllReplica / replicaNum;
- LOG.info("Update rows for table {} is {}, replicaNum is
{}, "
- + "rows for all replica {}, tablets count {}",
- olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
- statsStatus.updatedRows.addAndGet(tableUpdateRows);
- if (StatisticsUtil.enablePartitionAnalyze()) {
- updatePartitionRows(olapTable, tabletRows,
statsStatus, replicaNum);
+ Optional<Database> dbOption = catalog.getDb(event.getDbId());
+ if (!dbOption.isPresent()) {
+ LOG.warn("Database {} does not exist.", event.getDbId());
+ continue;
+ }
+ Database database = dbOption.get();
+ Optional<Table> tableOption =
database.getTable(record.getKey());
+ if (!tableOption.isPresent()) {
+ LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
+ continue;
+ }
+ Table table = tableOption.get();
+ if (!(table instanceof OlapTable)) {
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ TableStatsMeta statsStatus = findOrCreateTableStats(olapTable);
+ short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
+ Map<Long, Long> tabletRows = record.getValue();
+ if (tabletRows == null || tabletRows.isEmpty()) {
+ LOG.info("Tablet row count map is empty");
+ continue;
+ }
+ long rowsForAllReplica = 0;
+ for (Entry<Long, Long> entry : tabletRows.entrySet()) {
+ rowsForAllReplica += entry.getValue();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Table id {}, tablet id {}, row count {}",
+ record.getKey(), entry.getKey(),
entry.getValue());
}
}
+ long tableUpdateRows = rowsForAllReplica / replicaNum;
+ LOG.info("Update rows for table {} is {}, replicaNum is {}, "
+ + "rows for all replica {}, tablets count {}",
+ olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
+ statsStatus.updatedRows.addAndGet(tableUpdateRows);
Review Comment:
[P2] Exclude temporary-partition rows from the normal-table fallback
The tablet records also contain loads into temporary partitions:
PublishVersionDaemon.getBaseTabletIdsForEachBe resolves their IDs through
OlapTable.getPartition(long), which searches temporary partitions too. This
increment counts all of them, but an ordinary OlapScan and getRowCountForIndex
cover only normal partitions. On a never-analyzed range-partitioned DUP table,
load 5 rows into a normal partition and 100 using INSERT INTO t TEMPORARY
PARTITION(tp1) before the normal tablets' first report. The new record has
collected count/baseline 0 and updatedRows=105, so Project(*) -> OlapScan(t,
normal partitions) gets cardinality105 although its table contains only5
queryable rows. The later normal-only updatePartitionRows helper does not
correct this global counter. Keep temporary staging rows out of the
normal-table fallback, or derive that count from partition-specific deltas, and
cover a temporary load alongside an unreported normal partition.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/analysis/AnalysisManager.java:
##########
@@ -1366,52 +1405,52 @@ public void replayUpdateRowsRecord(UpdateRowsEvent
event) {
// Record : TableId -> (TabletId -> update rows)
if (event.getTabletRecords() != null) {
for (Entry<Long, Map<Long, Long>> record :
event.getTabletRecords().entrySet()) {
- TableStatsMeta statsStatus = idToTblStats.get(record.getKey());
- if (statsStatus != null) {
- Optional<Database> dbOption =
catalog.getDb(event.getDbId());
- if (!dbOption.isPresent()) {
- LOG.warn("Database {} does not exist.",
event.getDbId());
- continue;
- }
- Database database = dbOption.get();
- Optional<Table> tableOption =
database.getTable(record.getKey());
- if (!tableOption.isPresent()) {
- LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
- continue;
- }
- Table table = tableOption.get();
- if (!(table instanceof OlapTable)) {
- continue;
- }
- OlapTable olapTable = (OlapTable) table;
- short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
- Map<Long, Long> tabletRows = record.getValue();
- if (tabletRows == null || tabletRows.isEmpty()) {
- LOG.info("Tablet row count map is empty");
- continue;
- }
- long rowsForAllReplica = 0;
- for (Entry<Long, Long> entry : tabletRows.entrySet()) {
- rowsForAllReplica += entry.getValue();
- if (LOG.isDebugEnabled()) {
- LOG.debug("Table id {}, tablet id {}, row count
{}",
- record.getKey(), entry.getKey(),
entry.getValue());
- }
- }
- long tableUpdateRows = rowsForAllReplica / replicaNum;
- LOG.info("Update rows for table {} is {}, replicaNum is
{}, "
- + "rows for all replica {}, tablets count {}",
- olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
- statsStatus.updatedRows.addAndGet(tableUpdateRows);
- if (StatisticsUtil.enablePartitionAnalyze()) {
- updatePartitionRows(olapTable, tabletRows,
statsStatus, replicaNum);
+ Optional<Database> dbOption = catalog.getDb(event.getDbId());
+ if (!dbOption.isPresent()) {
+ LOG.warn("Database {} does not exist.", event.getDbId());
+ continue;
+ }
+ Database database = dbOption.get();
+ Optional<Table> tableOption =
database.getTable(record.getKey());
+ if (!tableOption.isPresent()) {
+ LOG.warn("Table {} does not exist in DB {}.",
record.getKey(), event.getDbId());
+ continue;
+ }
+ Table table = tableOption.get();
+ if (!(table instanceof OlapTable)) {
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ TableStatsMeta statsStatus = findOrCreateTableStats(olapTable);
+ short replicaNum =
olapTable.getTableProperty().getReplicaAllocation().getTotalReplicaNum();
+ Map<Long, Long> tabletRows = record.getValue();
+ if (tabletRows == null || tabletRows.isEmpty()) {
+ LOG.info("Tablet row count map is empty");
+ continue;
+ }
+ long rowsForAllReplica = 0;
+ for (Entry<Long, Long> entry : tabletRows.entrySet()) {
+ rowsForAllReplica += entry.getValue();
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Table id {}, tablet id {}, row count {}",
+ record.getKey(), entry.getKey(),
entry.getValue());
}
}
+ long tableUpdateRows = rowsForAllReplica / replicaNum;
+ LOG.info("Update rows for table {} is {}, replicaNum is {}, "
+ + "rows for all replica {}, tablets count {}",
+ olapTable.getName(), tableUpdateRows, replicaNum,
rowsForAllReplica, tabletRows.size());
+ statsStatus.updatedRows.addAndGet(tableUpdateRows);
+ if (StatisticsUtil.enablePartitionAnalyze()) {
+ updatePartitionRows(olapTable, tabletRows, statsStatus,
replicaNum);
+ }
}
return;
}
- // Handle truncate table
+ // Handle truncate table. A partition truncation is not a load: the
rows it reports are the rows it
Review Comment:
[P2] Invalidate the load-derived count when partitions are removed
A partition truncation also reaches records created by a load, but the
retained branch below adds the positive number of removed rows to updatedRows.
For a never-analyzed DUP table with two reported 50-row partitions, the new
record has a zero collected count/baseline and updatedRows=100. Truncating one
partition and inserting 5 rows into its replacement makes this counter 155
although the table has 55 rows. The first load marks the replacement index's
row count unreported, so an unpruned scan takes getRowCountWithDeltaRows and
uses 155 as cardinality. DROP PARTITION similarly retains the historical count:
dropping one 50-row partition, adding a replacement, and loading 5 yields 105
instead of 55 through the same fallback. Before this change an unanalyzed table
had no record to turn these modification counts into cardinality. Invalidate or
adjust the load-derived count when partitions are removed, including partial
TRUNCATE and DROP PARTITION, and cover removal followed by load.
--
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]
