Repository navigation
[refactor](changelog) Use explicit column mappings for reads and writes - #69585
Open
TsukiokaKogane wants to merge 1 commit into
Open
TsukiokaKogane wants to merge 1 commit into
TsukiokaKogane wants to merge 1 commit into
Conversation
TsukiokaKogane
requested review from
924060929,
deardeng,
englefly,
gavinchou,
liaoxin01,
luwei16,
morrySnow,
mymeiyi,
starocean999 and
yiguolei
as code owners
October 25, 2026 09:36
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
TsukiokaKogane
force-pushed
the
refactor_change_log_column_mapping
branch
from
October 25, 2026 09:38
9db0ae2 to
3411ce2
Compare
### What problem does this PR solve? Related PR: apache#67456, apache#66472 Problem Summary: Changelog execution inferred current/before relationships from column names and layout positions, coupling query projection to storage layout and leaving publish retries without a stable write-time relationship. Plan explicit slot pairs for reads and immutable source/current/before UID snapshots for writes. Register snapshots against actual transaction IDs, freeze them at precommit/commit, and retain them for replay and lagging-replica republish. Propagate the same snapshots through remote commit, pending publish persistence and Cloud transaction contexts. Resolve a schema-bound CID layout once per write/publish operation and keep query protocol parsing in Scanner. Validate mappings before visibility, preserve key-only historical lookup, and propagate pending-record save failures before queueing asynchronous work. ### Release note Changelog reads and writes use explicit column mappings. Changelog requests requiring mappings now reject missing or malformed mappings. Mixed-version adapters and rolling-upgrade behavior are outside this implementation scope. ### Check List (For Author) - Test: 85 targeted FE unit tests passed. FE and ASAN BE builds passed. FE Checkstyle, clang-format 16, header hygiene, changed-line clang-tidy and git diff --check passed. BE unit compilation was stopped at the user's request before execution; no BE unit pass result is claimed. Regression tests were not run at the user's request. Existing macOS ANN test portability adjustments used locally are excluded from this commit. - Behavior changed: Yes (explicit changelog protocol contract and transaction-bound recovery snapshots; ordinary writes do not require mappings). - Does this need documentation: No (local design and implementation notes are excluded from this code commit).
TsukiokaKogane
force-pushed
the
refactor_change_log_column_mapping
branch
from
October 25, 2026 09:39
3411ce2 to
fe2e078
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: two P1 regressions remain at head fe2e078: transactional VALUES into changelog tables fails through non-master FEs, and existing changelog tables created with light_schema_change=false lose write availability after upgrade. Both are explained inline. There were no existing inline threads to carry forward.
Static review covered all 88 changed files and their related FE/BE control flow. Three reviewers covered FE, BE, and transaction snapshot durability; the final unchanged finding set converged in round 2. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: explicit read slot pairs and transaction-bound write UID snapshots implement the intended identity contract on the checked normal paths. The two inline scenarios prevent the goal being met for all supported tables and FE roles. Added unit tests cover resolver validation, reordered tuples, immutable snapshots, replay, pending-publish persistence, queue rejection, and Cloud bitmap eviction; they miss these two scenarios.
- Scope and clarity: changes stay focused on mapping production, transport, resolution, and corresponding reader API/test updates.
- Concurrency: immutable snapshot replacement and synchronized status transitions freeze registration before PRECOMMITTED/COMMITTED persistence. BE queues use _async_publish_lock; transaction locks retain txn-before-map order, and normal/async publishers retain changelog-before-base tablet locking. Retry cleanup releases tablet locks before requeueing. No additional lock inversion was substantiated.
- Lifecycle and static initialization: layouts pin both schemas; synchronous derive contexts borrow the owned layout; queued/running/retry tasks retain mappings. No new global initializer or ownership cycle was found.
- Configuration: none added.
- Compatibility: new pending metadata is optional, but persisted old source UIDs are not migrated (second finding). Mixed-version adapters are explicitly excluded by the PR: an older remote writer omitting the new snapshot list is rejected even for an ordinary target. This declared limit is not separately reposted as an inline finding.
- Thrift/protobuf: every touched definition was compared with live-current branch-3.1, branch-4.0, branch-4.1, and branch-4.2. New IDs are unused there, new fields are optional/repeated, and no enum or existing field meaning is changed.
- Parallel paths: local/Cloud group writers, normal/async publish, recovery, bitmap-cache misses, remote commit, broker/stream/routine loads, actual-child republish, direct/vertical readers, compaction, and schema change were traced. The non-master VALUES exception is the first finding.
- Conditional checks: validation branches correspond to concrete malformed mappings/projections or failed persistence/ownership transfer. Existing VARIANT rejection excludes the suspected extracted-column layout mismatch.
- Test coverage: useful unit cases were added, but follower transactional VALUES and replay of old light-schema-change-disabled tables need coverage. No build, unit test, regression run, or runtime fault injection was performed under this static-only contract.
- Expected results: no regression .out files changed; new unit expectations were inspected against the relevant layouts and error paths.
- Observability: missing mappings report transaction/tablet context, queue submission failure logs tablet/version/status, and existing publish statistics remain. No additional metric requirement was established.
- Transactions and persistence: commit/precommit Gson state carries actual-write snapshots in both edit-log modes; replay rebuilds retained child aliases. Durable pending records retain the same mapping across restart and async retry; Cloud TxnVal preserves it independently of bitmap-LRU eviction.
- Writes and atomicity: layout validation precedes rowset visibility, and existing bitmap/dual-rowset ordering remains. No additional new crash/atomicity defect was substantiated on the checked paths.
- FE-BE variables: read pairs, sink schema thrift/protobuf forwarding, publish/republish snapshots, remote commit, Cloud attachment, and pending-record recovery are covered. The first finding identifies where registration fails before transport.
- Performance: layout resolution occurs per writer/publish operation; derive reuses resolved vectors and COW columns. No new rowwise name lookup, pathological retry scan, or significant allocation regression was substantiated.
- Other invariants: version visibility and MoW placeholder replacement are unchanged; nullable widening uses the existing helper, and error paths fail before publication. No additional verified issue was found in the final recheck.
Convergence: true, 2 rounds. Remaining limits: static analysis only; no runtime or build validation, and mixed-version behavior has the declared limitation above. No unresolved candidate or uncovered assigned scope remains.
Review converged after 2 round(s).
| taskInfo.isLoadToSingleTablet(), taskInfo.isStrictMode(), timeout, uniquekeyUpdateMode, | ||
| uniquekeyUpdateNewRowPolicy, partialUpdateInputColumns); | ||
| if (!Config.isCloudMode() && !(loadPlanInfo.olapTableSink instanceof GroupCommitBlockSink)) { | ||
| Env.getCurrentGlobalTransactionMgr().registerChangelogWriteMappings(dbId, taskInfo.getTxnId(), |
Contributor
There was a problem hiding this comment.
[P1] Register transactional VALUES snapshots on the master
In a shared-nothing cluster, connect to a follower/observer and run BEGIN; INSERT INTO t VALUES (...); COMMIT; against a changelog-enabled table. BatchInsertIntoTableCommand implements NoForward: InsertUtils.beginBatchInsertTransaction forwards only beginTxn to the master, then InsertStreamTxnExecutor plans locally. The master's BACKEND_STREAMING PREPARE state is deliberately not journaled (DatabaseTransactionMgr.persistTransactionState / enqueueTransactionState), so this new local registration throws Cannot register changelog mappings for missing transaction ... before execution. Forward the exact write snapshot to the transaction-owning master, or perform planning there; merely skipping follower registration would leave the required publish snapshot missing. Please cover explicit transactional VALUES through a non-master FE.
| Preconditions.checkState(before != null, "Missing before column: %s", source.getName()); | ||
| beforeUid = before.getUniqueId(); | ||
| } | ||
| entries.add(new Entry(source.getUniqueId(), current.getUniqueId(), beforeUid)); |
Contributor
There was a problem hiding this comment.
[P1] Migrate existing changelog source IDs before requiring unique UIDs
A changelog DUP table (k INT, v INT) created before this change with light_schema_change=false is valid, but both source columns retain UID -1: the old table UID initializer returned early, while only the companion index initialized its IDs. The new OlapTable.initSchemaColumnUniqueId guard fixes new creations, but its caller is the create-table path; neither OlapTable.gsonPostProcess nor MaterializedIndexMeta.gsonPostProcess migrates existing source IDs. After a full upgrade, every INSERT/load into that persisted table reaches this method and fails with Duplicate source UID: -1 in the constructor. Provide compatible identity migration/resolution for the existing FE/BE schemas before requiring unique source UIDs, and add coverage that loads an old catalog with this table property.
Contributor
TPC-H: Total hot run time: 28535 ms |
Contributor
TPC-DS: Total hot run time: 150609 ms |
Contributor
TPC-H: Total hot run time: 28561 ms |
Contributor
TPC-DS: Total hot run time: 151333 ms |
Contributor
ClickBench: Total hot run time: 25.26 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Changelog execution currently infers current/before column relationships from column names and physical positions. This couples read projection and write processing to the storage layout, while publish retries and recovery lack an explicit, transaction-bound mapping contract.
This PR introduces structured column mappings across FE and BE:
The change removes execution-time relationship inference, preserves key-only historical lookup and complete-row comparison for MIN_DELTA, and validates mappings before publication. Ordinary writes do not require changelog mappings. Mixed-version compatibility adapters are outside the scope of this PR.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)