Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the complete four-file diff at 1cb6c79. Two P2 findings need correction: normal grouped changelog loads never report their attached changelog rowsets, and the new Java import violates the mandatory FE Checkstyle gate. Both are posted inline. There were no existing inline findings to carry forward and no additional user-provided focus points.
The review converged in 2 rounds: all three independent reviewers returned NO_NEW_VALUABLE_FINDINGS in the latest round, every changed file and assigned mechanism was covered, and all candidates and initial risks were verified, deduplicated or dismissed with source evidence.
Critical checkpoint conclusions:
- Goal and proof: separate base/changelog load counters are the intended behavior. The FE accumulator works for directly supplied values, but the BE loop sees base transaction rowsets while the changelog resides in attach_changelog; therefore ordinary loads do not populate the new counter. The added test bypasses this producer and does not prove the complete behavior.
- Scope and reuse: the four-file change is focused and reuses the existing tablet delta report and statistics serialization.
- Concurrency and locking: the publish parent owns the report map and joins its workers; existing changelog-first tablet locks are unchanged. All accesses and lazy initialization of the new map take the TableStatsMeta monitor. No added IO, journal waits, or lock-order cycle was found. A repair must read the final attachment count after workers finish because MoW publication can append changelog rows.
- Lifecycle and initialization: TabletTxnInfo retains attachment ownership through publication and recovery. The new map belongs to the statistics record, survives ordinary analyze updates, and is discarded with that record. No new static initialization or reference cycle is introduced.
- Configuration: no configuration is added.
- Compatibility: the optional Gson field is initialized lazily when absent, following existing concurrent-map serialization patterns. The existing report field is reused; no new protocol shape or toolchain/library dependency is introduced. Older producers do not supply changelog history.
- Thrift/protobuf IDs: no definition is changed, so no field-ID or required-field compatibility change applies.
- Parallel paths: normal, retry, asynchronous and cloud reporting paths were traced. Existing replica averaging/missing-report approximations remain. Cloud/legacy events still report base table scalars and do not populate the new index counter; this is a feature limitation, with no separate current consumer failure established.
- Conditional logic: comments explain why changelog rows differ from base rows, but the BE condition tests the wrong transaction representation (first inline finding). Zero additions intentionally leave the map unchanged. Missing-tablet/checkpoint concerns were checked against actual index rebuild and replay registration, without another reachable regression.
- Test coverage: the new deterministic JUnit case covers additive map behavior, zero additions and base isolation. It adds no publish/report integration, routing, replay or serialization coverage. Existing attachment recovery tests were inspected. No builds, tests, Checkstyle execution, benchmarks or runtime validation were performed under the static-only contract.
- Expected results: the added direct-map assertions match their inputs; no regression output artifact is changed.
- Observability: existing tablet/transaction identifiers and report logging remain. No separate logging or metrics defect was substantiated.
- Persistence and failover: transaction delta maps already persist with VISIBLE state and use the same statistics application on replay; image/CREATE/TRUNCATE paths register changelog tablet metadata. The new map participates in statistics JSON/image persistence. No additional replay divergence was verified.
- Writes and atomicity: the patch changes statistics reporting and estimates, without changing rowset visibility, version selection, delete bitmap publication or committed data writes. No new data-loss or memory-safety issue was identified by source inspection.
- FE/BE passing: the existing callback and aggregation chain accepts tablet-keyed counts, but the attached changelog count is missing at its producer (first finding). No other new variable needs propagation.
- Performance: the new FE classification adds one inverted-index lookup per reported tablet on changelog tables and bounded synchronized map work. No additional current-head performance regression was substantiated.
- Other issues: the import-order finding is supported by error-severity CustomImportOrder, the Maven validate binding and the FE Checkstyle workflow. A suspected stream-wrapper null dereference was dismissed because the sole current consumer cannot reach that fallback with the wrapper's empty local partition map.
Coverage limits: this is a static review. The lifetime accumulator alone is not an exact retained-row or per-stream unconsumed-row count after TTL, statistics recreation or upgrade. The stream-estimation consumer described in dependent PR #69850 is outside this diff, so no end-to-end stream-plan improvement or failure is claimed here. There are no unresolved candidates or unread assigned source paths.
Review converged after 2 round(s).
| auto tablet_id = rowset_meta_ptr->tablet_id(); | ||
| if (_publish_version_req.base_tablet_ids.contains(tablet_id)) { | ||
| if (_publish_version_req.base_tablet_ids.contains(tablet_id) | ||
| || tablet->is_changelog_tablet()) { |
There was a problem hiding this comment.
[P2] Report the attached changelog rowset for grouped loads
Normal changelog loads commit only the base rowset into TxnManager: GroupRowsetBuilder::commit_txn stores the companion in txn_info_it->second->attach_changelog.rowset, and ChangelogRowsetBuilder::commit_txn only marks it committed. get_txn_related_tablets therefore exposes the base tablet to this loop, so the new is_changelog_tablet() branch never inserts the companion tablet ID or its rows. Those loads leave the new FE changelog counter at zero. Report the attached rowset under its own tablet ID after publish workers finish (MoW publication can append changelog segments), and cover the emitted report map in a publish-path test; the added direct-accumulator test cannot detect this omission.
There was a problem hiding this comment.
Fixed in 5bb280e. You are right that the changelog rowset is attached to the base tablet's transaction info rather than committed as a tablet of its own, so the is_changelog_tablet() branch never saw it; it is gone now. The publish path collects the attached rowset of each base tablet and reports it under its own tablet id after token->wait(), so a rowset that a merge-on-write publication extends with changelog segments is counted with its final row count.
| import org.apache.doris.catalog.ScalarType; | ||
| import org.apache.doris.catalog.Table; | ||
| import org.apache.doris.catalog.TableIf; | ||
| import org.apache.doris.catalog.TabletMeta; |
There was a problem hiding this comment.
[P2] Place TabletMeta after Tablet to pass FE Checkstyle
The new import precedes Tablet, but fe/check/checkstyle/checkstyle.xml enables alphabetical CustomImportOrder at error severity. fe/pom.xml binds the failing check to validate, and the FE Checkstyle workflow also runs it, with no applicable suppression. Move TabletMeta below Tablet so the required validation gate accepts this change.
There was a problem hiding this comment.
Fixed in 5bb280e: TabletMeta now follows Tablet in the import block.
1cb6c79 to
5bb280e
Compare
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
yujun777
changed the title
There was a problem hiding this comment.
Reviewed the complete four-file diff at exact PR head 5bb280e against base 374d87d. The intended goal is to carry the rows written to the attached changelog index into FE statistics so changelog scans can use a useful row estimate. The accumulator and its direct unit test cover the storage side, but two end-to-end paths still lose that information.
Critical checkpoint conclusions:
- Goal and proof: the FE accumulator correctly keeps changelog rows separate from base rows for a direct tablet-record event, but a partial BE publish retry drops already-published tablets from the final report, and cloud commits never populate the accumulator. The added unit test does not exercise either producer path.
- Scope and reuse: the changes are focused and reuse the existing tablet delta report, transaction publication, and TableStatsMeta persistence mechanisms.
- Concurrency and locking: BE publication tasks run concurrently and are joined by token->wait() before the rowset count is read; existing changelog-first tablet lock order is unchanged. TableStatsMeta additions and reads are synchronized. The retry lifecycle still loses a successful tablet's report.
- Lifecycle: TxnManager removes a successful tablet's txn entry during publish. The worker clears its delta map before retrying, so a later successful retry cannot rediscover that tablet's attached changelog rowset.
- Configuration: no configuration is added.
- Compatibility: the new Gson field is optional and lazily initialized, with no thrift/protobuf or wire-shape change.
- Parallel paths: local BE tablet-record reporting is updated, but the cloud commit path still reports only table-level base-index rows. Asynchronous tablet-stat refresh may eventually fill the gap, but the immediate post-load estimate remains zero.
- Conditional logic and data correctness: changelog rows are intentionally excluded from base-table updatedRows and partition base-index accounting. The two findings are statistics undercounting issues; rowset visibility and delete-bitmap publication are otherwise unchanged.
- Persistence and failover: the existing transaction delta map is persisted with visible transaction state and replayed through AnalysisManager; the new per-index map participates in TableStatsMeta JSON persistence. No separate replay or locking defect was verified.
- Testing and observability: the direct JUnit test covers additive accumulation, zero additions, and base-index isolation. No publish-path, retry, cloud, replay, or serialization integration test was added. Under the static-only contract, no build, test, Checkstyle, benchmark, or runtime validation was run. Existing identifiers and report logging remain sufficient for these paths.
- Performance: the new local classification adds one inverted-index lookup per reported tablet and bounded synchronized map work; no additional current-head performance regression was substantiated.
Existing inline threads 4422920699 and 4422920705 were verified as fixed on this head and are not reposted. There is no additional user-provided focus.
The review reached the round limit after the required final sweep. All changed files and main producer/consumer paths were statically covered and the two candidates below were independently verified, but the latest delegated sweeps were interrupted by the temporary model-capacity error, so the contract's all-subagent NO_NEW_VALUABLE_FINDINGS convergence condition was not evidenced.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The final static sweep covered all four changed files and verified both findings, but the required latest-round delegated NO_NEW_VALUABLE_FINDINGS evidence was unavailable because both round-2 child sweeps hit the temporary model-capacity error. No unresolved code candidate remains; convergence is false solely because the delegated convergence evidence could not be obtained before the three-round cap.
| // 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. |
There was a problem hiding this comment.
[P2] Preserve attached changelog rows across publish retries
If one tablet publishes successfully and a sibling tablet fails, this vector is local to the failed execute attempt. TxnManager::publish_txn removes the successful tablet's txn info, PublishVersionWorkerPool clears table_id_to_tablet_id_to_num_delta_rows before retrying, and PublishVersionDaemon discards reports from tasks with any error tablet. The final successful retry therefore contains only the previously failed tablet, so the already-published tablet's changelog rows are never added to TableStatsMeta. Preserve successful tablets' changelog counts across retries or report them from the final aggregation so a partial publish cannot permanently undercount the changelog index.
There was a problem hiding this comment.
The retry path here has the same guarantee as the base index delta rows, and I kept it that way rather than diverging:
- Within one
execute()the retry loop is inside the function and the collected changelog rowsets are declared before it, so a retry of the same attempt keeps the rowset of a tablet that already published (the rowset object stays valid, and its row count is the final one). - Across attempts the report is discarded as a whole:
PublishVersionDaemon.calculateTaskUpdateRowsskips a task that reports any error tablet, for the base delta rows as well as for these. So a tablet that published in a failed attempt contributes its delta with the next report, which is the periodic tablet report and includes the changelog index. That is the existing behaviour of the base index counter, and changing it means aggregating per-tablet reports across attempts on the frontend, which I would rather do for both counters in one change than only for the changelog one.
There was a problem hiding this comment.
Updated in 9fb10c4. 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.
| @@ -1525,6 +1535,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event) { | |||
| + "rows for all replica {}, tablets count {}", | |||
| olapTable.getName(), tableUpdateRows, replicaNum, rowsForAllReplica, tabletRows.size()); | |||
There was a problem hiding this comment.
[P2] Feed cloud changelog writes into the per-index counter
The new counter is updated only for local tabletRecords. Cloud commits call updateUpdatedRows(updatedRows, ...) with a table-level value from meta-service calc_table_stats, and that helper sums only base_tablet_ids, so attached changelog rowsets are never added. Immediately after a cloud load, before the asynchronous tablet-stat refresh reports the changelog index, getRowCountWithDeltaRows therefore returns zero for a changelog scan even though rows are visible. Extend the cloud commit/statistics path to report changelog rows per index (or carry them in the existing response) before relying on this accumulator.
There was a problem hiding this comment.
Agreed, and it is recorded as a known limitation in the description: a cloud commit reports rows per table from the meta-service calc_table_stats, which sums only base_tablet_ids, so there is no per-index value to route. Until that path carries the changelog rows, a cloud stream read keeps falling back to the row count of the table, which is the behaviour before this PR — safe, just loose. Feeding the cloud counter needs the meta-service side to sum the attached changelog rowsets as well; I would keep it in a follow-up rather than growing this one into the cloud txn path.
There was a problem hiding this 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 9fb10c4 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.
|
run buildall |
|
/review |
5bb280e to
fecb190
Compare
|
run buildall |
|
/review |
TPC-H: Total hot run time: 28398 ms |
There was a problem hiding this comment.
Reviewed the complete four-file diff at exact PR head fecb190 against base 374d87d.
The PR's goal is to carry rows written to attached changelog rowsets into the FE per-index statistics accumulator. The grouped-load producer fix is present, and the direct accumulator test is correct, but three lifecycle paths still make the new count incorrect:
TableStatsMeta(OlapTable)creates a zero changelog baseline. After DROP STATS (or another record recreation), existing changelog rowsets are still readable while the physical changelog index count is unknown.StatsCalculatorthen falls back togetRowCountWithDeltaRows, whose new branch returns only post-creationindexUpdatedRows, underestimating the visible changelog until a later tablet report.- Partial TRUNCATE PARTITION replaces changelog tablets but retains the table statistics record. The asynchronous
dropStatspath invalidates column/partition statistics without clearingindexUpdatedRows, so the branch continues returning rows from the removed partition. - A load whose partition is force-dropped before visibility can still carry old tablet delta rows.
DatabaseTransactionMgrremoves the missing partition from commit info, but still forwards the transaction delta map; after the old tablet metadata is removed,isChangelogTabletreturns false and this code routes those changelog rows into the baseupdatedRowscounter permanently.
Critical checkpoint conclusions:
- Goal and proof: the normal attached-rowset path now reports the final row count after
token->wait(), and FE routes direct tablet records by index. The new unit test covers additive accumulation and base-index isolation, but it does not cover record recreation, partition replacement, missing metadata, or the publish/report integration. - Scope and reuse: the four-file change is focused and reuses the existing tablet delta map and
TableStatsMetaJSON persistence. The three findings above are in the new counter's lifecycle and consumer boundary. - Concurrency and locking: BE publication workers are joined before the attached rowset metadata is read; existing changelog-first tablet lock order is unchanged. The FE counter methods synchronize on
TableStatsMeta, and no new deadlock or blocking I/O was found. Existing publish-retry aggregation remains a known P2 thread and is not duplicated here. - Lifecycle and initialization: the new accumulator has no partial-truncate reset and no pre-existing-row baseline, and the missing-metadata branch silently chooses the wrong index. No static initialization or ownership cycle was introduced.
- Configuration, Thrift/protobuf, ABI, and protocol compatibility: no configuration, generated definition, field ID, wire shape, or toolchain change is present.
- Parallel paths: local tablet-record reporting is updated. Cloud table-level reporting still omits changelog rows, and publish retries can lose successful siblings; both are already reported by existing inline P2 comments 4424460818 and 4424460813 and are not reposted. The grouped-load omission and import-order comments are fixed on this head.
- Error handling and data correctness: rowset visibility, version selection, delete bitmaps, and committed data writes are unchanged. The new failures affect statistics/cardinality accounting; unknown tablet metadata must not be silently treated as a base tablet.
- Persistence/failover: the optional Gson field participates in normal serialization, but stale or zero lifecycle state is what causes the findings above. Transaction delta maps are still applied through the existing visible/replay path.
- Testing and observability: only the added deterministic JUnit behavior is represented in the diff; no publish-path, replay, truncate, serialization, or cloud integration test was added. Under the static-only contract, I did not run builds, tests, Checkstyle, benchmarks, or runtime validation. Existing transaction/tablet identifiers and logging are sufficient for the reviewed paths.
- Performance: the new classification performs one inverted-index lookup per reported tablet and bounded synchronized map work; no separate current-head performance regression was substantiated.
- User focus: no additional user-provided focus points were supplied.
The review converged after 2 rounds. All three assigned round-2 rechecks returned NO_NEW_VALUABLE_FINDINGS; all changed files and assigned mechanisms were covered, and every candidate was verified, deduplicated, or dismissed. The review remains static-only, so runtime ordering of asynchronous reports is a stated coverage limit. Existing P2 threads 4424460813 and 4424460818 remain applicable to this head but are not duplicated. No existing P0/P1 inline finding applies, so existing_blocking_comment_ids is empty.
Review converged after 2 round(s).
| 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 |
There was a problem hiding this comment.
[P2] Preserve the existing changelog baseline when recreating table statistics. After DROP STATS (or another removal/recreation of TableStatsMeta), new TableStatsMeta(OlapTable) initializes no changelog row count. While the changelog tablet's FE row count is still unknown, StatsCalculator falls back to this branch, which returns only indexUpdatedRows from loads after the record was created. Existing changelog rowsets therefore remain visible but are estimated as zero/partial until a separate tablet report supplies a count. Preserve an existing changelog baseline or keep the fallback unknown instead of converting a missing baseline to zero.
There was a problem hiding this comment.
Fixed in 9fb10c4 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.
| return; | ||
| } | ||
| indexUpdatedRows().merge(indexId, rows, Long::sum); | ||
| } |
There was a problem hiding this comment.
[P2] Reset changelog deltas when a partition is truncated. Partial TRUNCATE PARTITION replaces the partition and its changelog tablets but keeps the table's TableStatsMeta; the subsequent dropStats invalidates column/partition statistics without clearing indexUpdatedRows. A later changelog row-count fallback therefore returns rows from the deleted partition as if they were still present. Clear or partition-scope this accumulator as part of the truncate/replay lifecycle so the estimate cannot retain removed rows.
There was a problem hiding this comment.
Fixed in 9fb10c4. 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.
| 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. |
There was a problem hiding this comment.
[P2] Do not route unknown tablet IDs into the base counter. When a load's partition is force-dropped before its transaction becomes visible, DatabaseTransactionMgr removes the missing partition from the commit info but still forwards the transaction's tablet delta map. The old changelog tablet metadata has already been removed, so isChangelogTablet returns false and this branch adds its rows to updatedRows. That permanently inflates the base estimate and leaves the changelog counter missing. Carry the index identity with the event or discard/handle missing tablet metadata explicitly instead of treating it as a base tablet.
There was a problem hiding this comment.
Fixed in 9fb10c4. 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.
TPC-DS: Total hot run time: 151672 ms |
ClickBench: Total hot run time: 24.36 s |
There was a problem hiding this comment.
Static review of PR 69896 at head fecb190 converged after three bounded rounds. Four new P2 findings are inline: the synthetic changelog wrapper returns a zero row estimate before the new TableStatsMeta fallback; user-injected base stats bypass the changelog accumulator; TTL compaction leaves the persisted accumulator stale; and per-partition replica allocations are divided by the table default. Existing review context was independently revalidated: stale/unknown tablet routing (r4425241009), partial-truncate reset (r4425241002), recreated-statistics baseline (r4425240994), and the cloud limitation (r4424460818) still apply and were not reposted; the retry concern (r4424460813) is shared pre-existing base-delta behavior; the checkstyle import issue (r4422920705) is fixed. No additional user focus was supplied. BE publication, FE routing/locking, lifecycle, serialization, and test coverage were inspected; no new issue was found beyond the inline set. This was a static-only review: builds and tests were not executed. All anchors were verified against the authoritative diff, and all final reviewers returned NO_NEW_VALUABLE_FINDINGS.
Review converged after 3 round(s).
| 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. |
There was a problem hiding this comment.
[P2] Route changelog scans through the new row-count fallback. StatsCalculator calls TableStatsMeta.getRowCountWithDeltaRows only when getRowCountForIndex(..., true) returns -1, but OlapChangelogTableWrapper inherits OlapTable.getRowCountForIndex without overriding it and its inherited idToPartition is empty, so that method returns 0. Every real changelog scan therefore bypasses this branch and is planned as an empty table despite indexUpdatedRows. Delegate the lookup to the origin table or override the wrapper so the accumulated changelog count is reachable.
There was a problem hiding this 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 9fb10c4 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.
| * 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) { |
There was a problem hiding this comment.
[P2] Keep the changelog accumulator reachable with injected stats. When userInjected is true, StatsCalculator returns getRowCount(selectedIndexId) without the fallback. ALTER STATS normally injects only the base index, so a changelog scan receives UNKNOWN and ignores the rows accumulated by this new field. Apply the changelog branch before the userInjected shortcut or otherwise supply a changelog-index value.
There was a problem hiding this comment.
The StatsCalculator shortcut for injected statistics is a consumer-side issue to address with explicit wrapper-estimate priority in #69850; this PR does not change that shortcut.
In 9fb10c4 the separate TableStatsMeta accumulator was removed. Changelog deltas now update the partition MaterializedIndex row count, which the P/Q metadata lookup reads independently of injected base statistics. This supplies the source count, while wrapper estimate selection remains in the consumer PR.
| */ | ||
| public synchronized void addIndexUpdatedRows(long indexId, long rows) { | ||
| if (rows <= 0) { | ||
| return; |
There was a problem hiding this comment.
[P2] Account for changelog TTL expiration in this accumulator. Each load is merged permanently into indexUpdatedRows, but BE changelog TTL compaction removes expired rowsets and no FE path subtracts or invalidates those rows. During the interval when the changelog index row count is still unknown, getRowCountWithDeltaRows reports expired rows as readable, overestimating the scan. Invalidate this fallback on TTL-driven removal or tie it to a current reported baseline.
There was a problem hiding this comment.
Fixed in 9fb10c4 by removing the lifetime accumulator. Loads now adjust the partition's changelog MaterializedIndex estimate, and the existing periodic tablet-stat path replaces that estimate with the retained row count, including TTL removal. The estimate can still be stale until a fresh report arrives; it no longer permanently accumulates expired rows in a separate persisted map.
The new test replaces the estimate with a lower reported retained count and verifies that the next load is added to that refreshed baseline.
| @@ -1525,6 +1535,7 @@ public void replayUpdateRowsRecord(UpdateRowsEvent event) { | |||
| + "rows for all replica {}, tablets count {}", | |||
| olapTable.getName(), tableUpdateRows, replicaNum, rowsForAllReplica, tabletRows.size()); | |||
There was a problem hiding this comment.
[P2] Use the partition's replica allocation for changelog deltas. This code divides every changelog tablet's aggregate by the table default replica count, but MODIFY PARTITION allows a changelog partition to use a different allocation. With default 3 replicas and a one-replica partition, nine emitted rows are recorded as 3, so update/delete-only loads permanently undercount the changelog estimate. Divide per tablet/partition (or carry the producer cardinality) instead.
There was a problem hiding this comment.
Fixed in 9fb10c4. Each changelog tablet's aggregate delta is divided by its owning partition's replica allocation before updating that partition's index estimate. It is no longer divided by the table default.
The new test uses a table default of three replicas with one- and three-replica partitions: reported totals of 9 and 18 add 9 and 6 rows respectively, while changelog rows remain excluded from the base counter.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#69850, apache#69896 Problem Summary: Stream P/Q snapshots read partition index row counts, but changelog load deltas were stored in a separate lifetime counter in table statistics. That counter was not read by the partition lookup, lost its baseline after DROP STATS, and retained removed rows after TTL or partial TRUNCATE. Apply published changelog deltas directly to the partition index row-count estimate. Periodic tablet statistics replace the estimate, and partition replacement naturally discards it. Normalize each changelog tablet's rows using its partition replica allocation and ignore late reports for removed tablets or partitions so changelog rows cannot enter the base counter. Remove the unused lifetime counter and cover the partition lookup and statistics/partition lifecycle in AnalysisManagerTest. Cloud commits and partial publish retries remain subject to the existing asynchronous statistics limitations. A report that already includes the load may temporarily cause an overestimate until the next report. ### Release note Shared-nothing loads update partition changelog row-count estimates before periodic tablet statistics arrive. ### Check List (For Author) - Test: Unit Test - AnalysisManagerTest: 28 passed - TableStatsMetaTest: 15 passed - FE Checkstyle validation passed - Behavior changed: Yes, publish deltas update partition changelog estimates - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed exact head 9fb10c4c0e147d4b9b079b4e57961ab39d03bdaf. One new P2 finding is inline: unconditional companion rows change the semantics of a base-only report consumed by older FEs during supported BE-before-FE upgrades. No existing P0/P1 finding remains to carry forward.
The reviewed flow is grouped load -> attached changelog rowset -> publish workers/MoW append -> token wait -> tablet delta report -> transaction visibility/replay -> partition MaterializedIndex -> public row-count lookup.
Critical checkpoint conclusions:
- Goal and evidence: the same-head shared-nothing path supplies final changelog counts to partition estimates. The added FE test's assertions cover both public getters; full transport/P/Q integration is not tested here.
- Scope and reuse: the production change is focused and reuses existing reporting and partition index state.
- Concurrency and locking: master visibility and follower/checkpoint transaction replay hold table write locks, excluding TabletStatMgr's read-locked count replacement. The coordinator reads rowset counts after worker completion. No new lock order or heavy locked operation was found.
- Lifecycle/static initialization: shared ownership retains attached rowsets through reporting; DROP STATS preserves index counts and partition replacement discards the affected baseline. Checkpoint loading/replay rebuilds tablet metadata. No new static initializer, cycle or leak was found.
- Configuration: none added.
- Compatibility: existing storage fields remain usable, but reused report semantics are incompatible with older FE readers; see the new P2.
- Thrift/protobuf IDs: no definitions changed, so cross-branch field-ID comparison is not applicable.
- Parallel paths: cloud commits and async/cross-attempt publication still depend on later tablet statistics for omitted changelog deltas. Existing threads already cover cloud and retries; they are not reposted. Wrapper delegation and injected-stat priority remain consumer work discussed in #69850.
- Conditions: attachment-null, missing-tablet and removed-partition branches have concrete cases and nearby rationale; partition-specific replica allocation is used for changelog rows.
- Test coverage: inspected positive and negative FE assertions cover mixed one-/three-replica partitions, missing metadata, DROP STATS, baseline replacement and partial TRUNCATE with late reports. BE emitted-map/MoW integration, mixed-version and end-to-end consumer tests remain absent.
- Test results: no result artifacts changed. This was static inspection only: no builds, tests, benchmarks or runtime validation were executed.
- Observability: existing publish and delta-report logs provide transaction/tablet/count context; no additional critical gap was found.
- Transaction/persistence/failover: visible transaction state carries the delta map and replays through the same path; MaterializedIndex.rowCount already persists. No new journal record is required. Counts remain approximate across independent statistics samples and failover.
- Data writes/atomicity/crashes: rowset visibility, versions and delete-bitmap transitions are unchanged; the new state is an estimate. Known failed-attempt omissions and stale/double-counted baselines do not establish an exact count or upper bound.
- FE/BE propagation: same-head transport reaches the correct index and keeps changelog rows out of base/partition update counters; mixed-version propagation is the inline defect.
- Performance/memory: additional work is linear in reported tablets with one metadata lookup and a shared pointer per companion; no material new retention or performance defect was found.
- Further sweep: all three changed files, upstream/downstream paths, candidate dispositions and risk items were rechecked. No other verified new finding or unresolved static candidate remains.
Existing review context: attached-rowset reporting and import ordering are fixed. The revised partition-index design addresses the reported replica-allocation, missing-tablet, DROP STATS, TRUNCATE and lifetime-TTL accumulator issues. Retry/cloud omissions remain acknowledged limitations; wrapper/injected-stat concerns remain consumer follow-ups. All existing findings are P2, so existing_blocking_comment_ids is empty. There was no additional user-provided focus.
Static review converged in 2 rounds. All assigned reviewers completed the final pass without additional valuable findings. Coverage limits are the unexecuted integration/upgrade behavior and absent tests described above; all changed paths and static risk questions were covered.
Review converged after 2 round(s).
| } | ||
| for (const RowsetSharedPtr& changelog_rowset : attached_changelog_rowsets) { | ||
| const RowsetMetaSharedPtr& rowset_meta = changelog_rowset->rowset_meta(); | ||
| tablet_id_to_num_delta_rows.insert({rowset_meta->tablet_id(), rowset_meta->num_rows()}); |
There was a problem hiding this comment.
[P2] Negotiate changelog deltas before extending the base-only report
The supported rolling-upgrade order upgrades BE before FE. During that window, a pre-PR FE's AnalysisManager.replayUpdateRowsRecord sums every entry in this existing map into base updatedRows, without classifying the new changelog tablet IDs. An insert producing 10 base rows and 10 changelog rows on each of three replicas therefore records 60 / 3 = 20 base delta rows instead of 10, inflating the base statistics counter and any row-preserving estimate that uses its delta. The new FE classifier cannot protect the old master, and PublishVersionTask.toThrift() sends no capability indicating that the receiver understands companion rows. Gate this reporting on a negotiated capability, or carry changelog deltas in a separate optional field that old FEs ignore, and cover the old-reader/new-writer case.
There was a problem hiding this comment.
Changelog has not been officially released yet, and there are no changelog tablets in production. Therefore, the rolling-upgrade compatibility scenario described here does not arise. Compatibility between unreleased changelog implementations is outside the scope of this PR.
TPC-H: Total hot run time: 28271 ms |
TPC-DS: Total hot run time: 150930 ms |
ClickBench: Total hot run time: 24.3 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#69896 Problem Summary: When partition analysis is enabled, changelog companion IDs remain unmatched in the base-tablet accounting loop. Each remaining ID is probed against every partition while the transaction holds the table write lock. Collect accepted base entries separately when partition analysis is enabled and pass only that map to partition accounting. Preserve the original transaction report for replay. Extend the existing lifecycle test to enable partition analysis, check base partition counts, and verify that an older unrelated partition is not probed after the loaded base tablet is matched. Also clarify that attached rowsets are retained for one publish attempt, not across publish retries. ### Release note Avoid unnecessary partition scans when loading changelog tables with partition analysis enabled. ### Check List (For Author) - Test: Unit Test - AnalysisManagerTest: 28 passed - FE Checkstyle validation passed - C++ format check passed - Behavior changed: Yes, partition accounting excludes changelog tablet IDs - Does this need documentation: No
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69896
commit: df4dea79118b3af97bf92cc1200b522859da0b12
base: 374d87d81de557a8b002cb94a5816b7d8d650c12
reviewed_at: 2026-11-04T11:32+08:00
reviewer: yujun777
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
### What problem does this PR solve? Issue Number: None Related PR: apache#69850 Problem Summary: A table stream estimates the change rows a read returns from the row count of the partition's changelog index: a read of the range between the stream offset and the end of the read returns at most the change rows the changelog still holds. The count the frontend holds comes from the tablet reports, which a backend sends every tablet_stat_update_interval_second (60s by default), so a read which follows a load within that interval sees the count from before the load. The estimate falls back to the row count of the table, which is safe but loose: it answers with the whole table where the answer is a few change rows. The frontend cannot derive the change rows from the rows it loaded into the base index: 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. The backend knows them, so it now reports them. - The publish task reports the delta rows of the changelog tablets as well, not only of the base tablets: the report already visits the rowsets of every tablet a transaction wrote, and a changelog tablet is one of them. - AnalysisManager.replayUpdateRowsRecord routes those tablets to the table record by the index of each tablet (from the tablet inverted index, which the followers have as well, so a replay reproduces the routing without a new journal entry), and TableStatsMeta accumulates them per index (Gson field "iur"). The base index rows are counted as before: the change rows are neither added to them nor mistaken for them. - TableStatsMeta.getRowCountWithDeltaRows returns the accumulated rows for a changelog index: an analysis never collects its row count, so the rows of the loads are the count there. A stream read consumes this through TableStatsMeta.getRowCountWithDeltaRows, which is where the estimate of PR apache#69850 reads its counters from; until that lands, the recorded rows are visible in the table statistics only. ### Release note None ### Check List (For Author) - Test: Unit test - Unit test: TableStatsMetaTest, a new case (the rows a load writes to the changelog index are accumulated per index and reported by getRowCountWithDeltaRows, while the base index rows are not affected). TableStatsMetaTest 16/16 and AnalysisManagerTest 27/27 pass with the change. - No regression exercises the reporting window: it needs a load followed by a read within the tablet reporting interval, which the suites cannot time reliably. Every load now reports the change rows as well, so the existing suites which read the row counts of a loaded table cover the base index side of the change. - Behavior changed: No user visible change; a load additionally reports the rows it wrote to the changelog index. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69850, apache#69896 Problem Summary: Stream P/Q snapshots read partition index row counts, but changelog load deltas were stored in a separate lifetime counter in table statistics. That counter was not read by the partition lookup, lost its baseline after DROP STATS, and retained removed rows after TTL or partial TRUNCATE. Apply published changelog deltas directly to the partition index row-count estimate. Periodic tablet statistics replace the estimate, and partition replacement naturally discards it. Normalize each changelog tablet's rows using its partition replica allocation and ignore late reports for removed tablets or partitions so changelog rows cannot enter the base counter. Remove the unused lifetime counter and cover the partition lookup and statistics/partition lifecycle in AnalysisManagerTest. Cloud commits and partial publish retries remain subject to the existing asynchronous statistics limitations. A report that already includes the load may temporarily cause an overestimate until the next report. ### Release note Shared-nothing loads update partition changelog row-count estimates before periodic tablet statistics arrive. ### Check List (For Author) - Test: Unit Test - AnalysisManagerTest: 28 passed - TableStatsMetaTest: 15 passed - FE Checkstyle validation passed - Behavior changed: Yes, publish deltas update partition changelog estimates - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69896 Problem Summary: When partition analysis is enabled, changelog companion IDs remain unmatched in the base-tablet accounting loop. Each remaining ID is probed against every partition while the transaction holds the table write lock. Collect accepted base entries separately when partition analysis is enabled and pass only that map to partition accounting. Preserve the original transaction report for replay. Extend the existing lifecycle test to enable partition analysis, check base partition counts, and verify that an older unrelated partition is not probed after the loaded base tablet is matched. Also clarify that attached rowsets are retained for one publish attempt, not across publish retries. ### Release note Avoid unnecessary partition scans when loading changelog tables with partition analysis enabled. ### Check List (For Author) - Test: Unit Test - AnalysisManagerTest: 28 passed - FE Checkstyle validation passed - C++ format check passed - Behavior changed: Yes, partition accounting excludes changelog tablet IDs - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69850 Problem Summary: Changelog index row counts can remain stale or unknown until periodic tablet statistics arrive. Base-table load rows cannot describe changelog writes: updates and deletes have their own physical changelog records. Cloud commit responses previously carried no partition/index-level changelog delta, and default cloud replica zeros could be marked as fully reported counts. This PR completes physical changelog row-count collection in shared-nothing and cloud deployments: - Read attached changelog rowsets after publish workers finish, including segments appended by merge-on-write publication. Normalize shared-nothing deltas by each partition's replica allocation and filter removed or replaced tablets. - Return optional partition/index-level changelog deltas from ordinary, lazy and subtransaction cloud commits, with tablet identities and the final partition version. Only completed visible commits supply additive statistics. Cloud rows are counted once without replica division and remain separate from base updatedRows. - Preserve unknown counts on zero deltas and establish positive partial estimates without setting the complete-report flag. Apply duplicate and out-of-order cloud responses once using compact version ranges; complete periodic reports retire superseded versions. - Overwrite retained counts from valid periodic tablet statistics so TTL can lower the count. Preserve partial-report validity and serialize count publication with commit updates under the table metadata lock. Replacing tablet data resets its local estimate; DROP STATS preserves it. Counts are physical records; UPDATE records are not multiplied by two. Periodic reports and commit deltas can temporarily overlap or lag. Failed publish attempts can miss immediate deltas, which later tablet reports calibrate; these estimates are not exact historical counts or strict upper bounds. ### Release note Successful loads update partition changelog row-count estimates in both shared-nothing and cloud deployments before the next periodic tablet report. ### Check List (For Author) - Test: Unit Test / Regression test - 121 focused FE tests covering analysis statistics, index counts, periodic reports, cloud commit callbacks and retries passed across focused runs. - ASAN BE publish test passed with real concurrent MoW writes, publish-time changelog segment growth, final row-count reporting and publish retry. - 5 ASAN meta-service RPC tests passed for ordinary, lazy and subtransaction commits, failures/retries, final temporary rowset metadata and base/changelog separation. - Shared-nothing changelog_p0 regression directory: 27 suites passed. The new suite pauses periodic statistics publication and checks immediate counts, stream results, DROP STATS and TRUNCATE. - FE build (excluding unavailable OBS dependencies), ASAN BE build, Checkstyle, clang-format, clang-tidy, build hygiene and cross-release protobuf checks passed. - Cloud end-to-end cluster regression was not run; cloud coverage consists of the RPC and FE tests above. - Behavior changed: Yes, successful loads immediately supplement partition changelog row-count estimates in both storage modes, while periodic statistics continue to calibrate retained stock. - Does this need documentation: No
df4dea7 to
740771a
Compare
yujun777
changed the title
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#69896 Problem Summary: Cloud replica row-count validity is transient, while checkpoints retain the last fetch time and adaptive polling interval. After FE recovery an idle tablet at the infinite interval was never fetched again, leaving base and changelog index counts incomplete. Fetch replicas without a valid report before applying the persisted backoff. A failed fetch remains eligible for retry; a successful report restores completeness and resumes normal backoff. ### Release note Cloud tablet row counts become available after FE recovery even for idle tablets that reached the maximum statistics polling interval. ### Check List (For Author) - Test: Unit Test - The new mode-2 recovery regression failed before the fix (no fetch request) and passed after it. - All 5 TabletStatMgrTest cases passed, including positive restored base/changelog counts, infinite/unexpired intervals, retry after a failed fetch, successful revalidation and resumed backoff. - Java incremental compilation and Checkstyle passed. - Behavior changed: Yes, replicas without a valid row-count report are fetched before polling backoff applies. - Does this need documentation: No
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69896
commit: e8ec7a37192a39f3ec326b9e41fc4616d7ca384d
base: ccfd4378d424292a363a96142ad795b8c9ace21e
reviewed_at: 2026-11-13T16:30+08:00
reviewer: yujun777
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers None. Reviewed locally with the |
|
run buildall |
### Background Changelog row-count estimates combine commit deltas with periodic tablet statistics. Cloud commits have ordinary, lazy and subtransaction paths. ### What problem does this PR solve? Issue Number: None Related PR: apache#69896 (follow-up verification) PR Type: Test Problem Summary: Verify cloud commit counts independently of periodic publication and remove five unnecessary runtime log statements. Make the immediate-count suite's stream cleanup work in cloud mode. ### Details - **Scenarios:** Multi-partition and multi-tablet INSERT SELECT subtransactions, commit, rollback and a subsequent commit; ordinary and forced-lazy immediate counts on a local ASAN cloud cluster. - **What existing tests miss:** RPC and FE unit tests did not exercise the full cloud SQL-to-MS-to-FE count path, and the immediate suite's cleanup lacked the FORCE required in cloud mode. - **Fails when broken:** With periodic publication paused, SHOW DATA must match physical changelog rows: 4 after the first subtransaction commit, 4 after rollback and 5 after the next commit. ### Testing - Cloud immediate-count and subtransaction suites passed in ordinary and forced-lazy runs. Existing MS logs confirmed commit_txn_eventually; the fuzzy configuration was restored to zero. - Cloud changelog_p0: 24/28 suites passed. Four existing row-content suites failed again in an isolated batch: test_changelog_before_column_name, test_cloud_changelog_publish_conflict, test_changelog_multi_segment and test_changelog_basic. Their golden files were not changed. - The immediate-count suite also passed in shared-nothing mode after the FORCE cleanup change. - FE and ASAN BE/MS builds, Java Checkstyle, C++ formatting and targeted MS clang-tidy passed after logging cleanup. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: the four cloud row-content suites listed above remain failing; the new count coverage passes. ### AI Disclosure - Tool: Codex (model not stated) - Generated: Logging cleanup and cloud regression coverage. - Human review: Not stated.
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69896
commit: a22b63e8cae0e60ca5874f230bc6e0848607429b
base: ccfd4378d424292a363a96142ad795b8c9ace21e
reviewed_at: 2026-11-13T18:03+08:00
reviewer: yujun777
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
### Background Visible loads supplement changelog index counts before periodic tablet statistics. Stream consumption advances its own offset independently of those current counts. ### What problem does this PR solve? Issue Number: None Related PR: apache#69850 (stream consumption baselines) PR Type: Test Problem Summary: Cover successive loads and stream consumptions while periodic row-count publication is paused. Current physical counts must grow after each load and remain unchanged by consumption; each successful consumption must empty its interval. ### Details - **Scenarios:** Extend the existing SN/cloud immediate-count suite with two load/consume cycles. Generated golden sections show retained counts 2, 3 and 4, the two new rows in the sink, and empty intervals after consumption. - **What existing tests miss:** The existing paused-report test performed only SELECT from the stream, without advancing its offset between writes. - **Fails when broken:** A missing immediate delta changes the recorded index count; failed offset advancement leaves a nonempty interval or repeats rows in the sink. ### Testing The extended suite passed on local shared-nothing and ASAN cloud clusters against the same runner-generated golden file. No production code changed in this commit. Four previously reported cloud row-content failures elsewhere remain outside this verification. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex - Generated: Regression additions, generated golden output, and this description - Human review: not stated
### Background Table streams read changelog ranges or reconstruct snapshots at a consumption offset. Optimizer statistics originate from base and changelog indexes and can lag data visibility. Each stream needs its own consumption-bound baseline independently of current index statistics. ### What problem does this PR solve? Issue Number: None Related PR: apache#62058 (introduced the OLAP wrapper), apache#69896 (supplements current changelog counts after visible loads) PR Type: Bugfix Problem Summary: Stream wrappers can size large reads as one row or use retained changelog stock as an incremental range. Estimate each read from its selected partitions, actual scan type and consumption-bound counts, with lifecycle checks and ordinary statistics fallback. Store the consumed offset, base count and changelog count in one partition state, prepare that state before transaction persistence, and publish the same tuple only after success. Shared-nothing transactions and cloud MS offsets persist the tuple through their existing storage paths. ### Details - **Change map:** production +790/-40, tests +1266/-23, generated golden output +44/-0, no repository documentation changes. Production changes cover stateless estimate formulas and wrapper/statistics plumbing; one persisted partition consumption state and source lifecycle binding; existing local transaction and cloud creation/commit integration; and nine optional protobuf fields with atomic MS offset storage/readback. Tests cover estimates, lifecycle/concurrency, transaction replay/abort and cloud/MS storage boundaries. - **Frequency:** Affected wrapper reads can collapse to one row; range quality depends on available statistics. Nonempty offsets with stale, zero or negative deltas require a stock fallback instead of proving zero rows. - **Root cause:** Wrappers inherited empty partition maps, while retained index counts do not describe a consumed range. Separate or post-commit baseline sampling can lose the association between the offset and its sampled counts. - **Why existing tests missed it:** Existing tests checked metadata delegation, SQL results and offset advancement without planner count assertions or durable offset/count tuples. Cloud transaction fixtures used unregistered streams and did not cover nonempty production capture/publication, report changes during RPCs, or the fair-lock cycle reproduced during review. - **Fix:** Persist offset/B0/C0 together in PartitionStreamState and pending OlapTableStreamUpdate. Sample before persistence/RPC; publish unchanged samples after success, leave failed/aborted updates unpublished, and retain counts for unchanged consumed offsets. Use positive changelog/base deltas, then stock/table fallback; only offsets prove an empty range. Apply actual scan-type expansion once and preserve reset/snapshot branch scales. Source sampling and cloud publication use nonblocking metadata locks; publication takes no source lock. FE/data lifecycle bindings invalidate stale estimates without discarding persisted numbers. - **Other paths and branches:** Covers SN/cloud creation, consumption, image/replay, MS latest/versioned offset storage and existing read states. First initial-snapshot consumption can transition to consumed at the same numeric offset. Column statistics and BE/MS current-count collection remain separate; apache#69896 updates current C1 without changing stream baselines. Stream/IVM storage is unreleased, so no old-format JSON migration is added. All new protocol fields are optional with unused IDs; stream messages are absent from branch-3.1/4.0/4.1/4.2. - **Behavior change:** Stream scan estimates use the consumed range and selected partitions. Offset and sampled counts are durable together; unknown or invalid baselines use existing fallback rules. Counts remain approximate, and completely unknown inputs retain ordinary OLAP handling. ### Testing - Local FE build and Checkstyle passed. 144 distinct focused FE tests passed across eleven classes, including actual TransactionState serialization/replay and abort, cloud registered-source commit/create hooks, unchanged offsets, first full consumption at an unchanged numeric TSO, and a nonempty range between reports. - ASAN Cloud build passed. All 42 MetaServiceTableStreamTest cases passed, covering offset/count storage and readback, restart, failed/CAS-conflicting commits, idempotence, unchanged consumed offsets and latest/versioned storage. Clang-format v16 and changed-line clang-tidy passed. - All 17 local SN stream regression suites passed, including planner estimates and ordered query results. The three prior concurrency reproductions failed before their fix and passed afterwards. The ASAN BE build passed earlier; no BE execution source changes are included. - Cloud FE paths used mocked MS RPCs and real in-memory MS transaction tests; this PR's new baseline protocol was not exercised on a live cloud cluster. The companion apache#69896 paused-report regression passed in SN and cloud with two successive load/consume cycles, against runner-generated output. ### Notes for Reviewers C0 is retained physical changelog stock, not the number of consumed rows. Initial full reads use base scan estimates. Replay preserves stored counts but clears their transient FE/source binding until a later successful consumption establishes one. Periodic/current-count updates never write stream baselines; report/delta overlap and lag remain approximation limits. ### Release note Table stream scans retain useful row estimates for incremental and snapshot reads, and persist sampled consumption counts together with their offsets. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex; gpt-6.1-sol/xhigh for independent reviews - Generated: Code, tests, and this description - Human review: not stated
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69896
commit: b352751e678f173ebe8140c245029708ee2963f7
base: ccfd4378d424292a363a96142ad795b8c9ace21e
reviewed_at: 2026-11-13T19:48+08:00
reviewer: yujun777
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
### Background Stream updates and scan wrappers keep partition offsets together with consumption counts or read metadata. Their parallel maps share partition IDs, but key sets differ for first consumption, empty partitions, cloud UNKNOWN states and optional scan statistics. ### What problem does this PR solve? Issue Number: None Related PR: apache#69896 (current changelog row counts) PR Type: Refactor Problem Summary: Store each partition's related update or scan fields in one map instead of separate maps. Preserve absent previous offsets, ranges, baselines, fixed versions and estimates, including their distinct projected key sets. ### Details - **Change map:** production +204/-183 and tests +118/-17. No protocol, generated output or repository documentation changes. Three state containers and their consumers are grouped by partition; tests check distinct field memberships, persistence, merge and identity. - **Moves vs. logic changes:** Replace two update maps with a map of previous offset and next complete state. Replace three maps in each wrapper with one map of partition metadata. Adapt consumption and publication to the grouped representation; keep existing merge, CAS, estimate and identity behavior. - **Behavior change:** No SQL or estimation behavior changes. Explicit row counts remain excluded from wrapper equality/hash; offset-only and version-only partitions are excluded from estimated totals. - **Compatibility:** The unreleased stream update JSON uses a single updates field. No migration for intermediate unreleased formats. FE/MS and FE/BE protocols are unchanged. - **Performance impact:** One stored map per class. Preserve direct O(1) partition lookups and cached presence flags. Existing map interfaces use lazy views rather than copying complete maps on every call. No benchmark claim. - **Rollback:** Revert this refactor before release; intermediate unreleased JSON is not migrated. ### Testing 199 focused FE tests across thirteen classes passed, including new mixed previous-offset merge/serialization/CAS tests, empty/UNKNOWN read memberships, disjoint scan metadata fields and estimate-free identity. Local FE build and Checkstyle passed. All 17 existing SN stream regression suites passed against unchanged golden files. No native source or protocol changes in this commit. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex; gpt-6.1-sol/xhigh for independent reviews - Generated: Refactor, tests and this description - Human review: not stated
### Background Stream updates and scan wrappers carry per-partition offsets, row-count baselines or read metadata. Their optional fields have different memberships, so one partition state must preserve which fields exist. ### What problem does this PR solve? Issue Number: None Related PR: apache#69896 (current changelog counts) PR Type: Refactor Problem Summary: Use named update values and one state map at construction boundaries. Remove split-map constructor overloads, use compact persisted names, and express initial/restored consumption states through explicit factories. Preserve read memberships, offset CAS and estimate-free wrapper identity. ### Details - **Change map:** Production +143/-120, tests +174/-128; no protocol or generated output changes. Three state APIs and their construction/consumption callers; tests adapt fixtures, check compact JSON and request an early BE tablet report before polling FE counts. - **Moves vs. logic changes:** Replace map-value Pair with immutable PartitionUpdate(previousOffset,nextState), and serialize it under u/p/n. Wrapper/update constructors accept one named-state map. PartitionStreamState has one complete internal constructor, an immutable initial seed and an explicit restored-fields factory; existing transitions preserve epoch/count semantics. - **Behavior change:** SQL behavior and read estimates are unchanged. The unreleased update JSON uses shorter names. The regression waiter requests an SN BE tablet report before its existing FE statistics condition. - **Compatibility:** No migration for intermediate unreleased stream JSON, as the feature is not externally released. All Java construction callers are adapted; FE/BE and FE/MS protocols are unchanged. - **Performance impact:** Retain expected-size allocation in extraction, update construction/commit copying and scan-state assembly. Immutable initial states share an empty seed; map views avoid full copies for existing read interfaces. No benchmark claim. - **Rollback:** Revert this refactor before release; intermediate unreleased JSON is not migrated. ### Testing Local FE build, Checkstyle and all test-source compilation passed. 199 focused FE tests across thirteen classes and 17 existing SN stream regression suites passed against unchanged golden files. Tests cover JSON short fields plus type discriminator, merge/roundtrip/CAS, initial and replay states, wrapper membership/identity, and planner/transaction/cloud creation and commit callers. ### Release note None ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex; gpt-6.1-sol/xhigh for independent reviews - Generated: Refactor, tests and this description - Human review: not stated
Background
Changelog indexes retain physical change records for table streams. Their row-count estimates come from periodic tablet statistics, independently of base-table load statistics. Updates and deletes can produce changelog records whose counts differ from the base-table load rows.
What problem does this PR solve?
Issue Number: None
Related PR: #69850 (persists consumption baselines with offsets and estimates stream ranges)
PR Type: Enhancement
Problem Summary:
Changelog row counts can remain stale or unknown between tablet reports, and cloud commit responses previously carried no partition/index-level changelog delta. Successful visible commits now supplement these estimates in both storage modes, while valid periodic reports calibrate retained stock. Most of the diff tests publication, retries, incomplete reports and recovery; no new configuration is required after upgrading.
Details
Testing
122 focused FE tests passed locally across runs, covering index counts, publication, retries and restored-cloud polling. The recovery scheduling test failed before the fix and passed afterwards.
One ASAN BE test passed with real concurrent MoW publication and retry; five ASAN MS RPC tests passed for ordinary, lazy and subtransaction commits, including failures and base/changelog separation.
Shared-nothing
changelog_p0: 27 suites passed before the final logging cleanup and cloud test additions; the immediate-count suite passed again after its cloud-compatible FORCE cleanup change.Local ASAN cloud cluster (1 FE, 1 BE, MS, Recycler and FDB): immediate-count and subtransaction suites passed with periodic publication paused; forcing lazy commits passed both suites against the same golden files, with existing MS logs confirming
commit_txn_eventually. The fullchangelog_p0directory passed 24/28 suites. Four existing suites failed on row contents:test_changelog_before_column_name,test_cloud_changelog_publish_conflict,test_changelog_multi_segment, andtest_changelog_basic; all four failed again in an isolated batch, and their expected results were not changed.With periodic reports paused, the extended immediate-count suite passed in SN and cloud: two successive loads and stream consumptions grow retained changelog counts from 2 to 3 to 4; consumption leaves current counts unchanged, and each consumed interval becomes empty. The new golden sections were generated by the runner.
Notes for Reviewers
updatedRows.DROP STATSpreserves the estimate; data replacement resets it. Complete periodic reports retire covered deduplication versions.Release note
Successful loads supplement changelog row-count estimates before the next periodic tablet report in both storage modes. Periodic statistics continue to calibrate retained counts.
Check List (For Author)
AI Disclosure