Conversation
The field is the high part of the sequence values an IVM MV's rows are stamped with -- the low part next to it is a delta index -- but "refreshVersion" reads like an epoch or like the MV's own version, which is exactly what the per-partition refreshEpoch being added alongside it is not. Nothing about the value changes: it still counts committing IVM transactions and still prefixes the sequence column. Key changes: - Rename the field and its accessors to sequencePrefix / getSequencePrefix / advanceSequencePrefix, and MTMV.getNextRefreshVersion to getNextSequencePrefix - Rename the identifiers in IvmSequenceCalculator to match, including LARGEINT_SEQUENCE_PREFIX_SHIFT and the range-check messages - Persist it as "sp" instead of "rv": IVM is not released, so there is no image or journal that writes the old name Unit Test: - IvmInfoTest.testSequencePrefixIsPersistedAsSp pins the persisted name and that "rv" is gone - IvmInfoTest / IvmSequenceCalculatorTest / IvmAggDeltaHandlerTest / IvmDeltaRewriteStateTest / DatabaseTransactionMgrTest cover the renamed API
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Static review result: one terminology-completeness issue remains; no runtime correctness defect was found.
- Goal and scope: the production change is behavior-preserving after normalizing the renamed identifiers. The accessor, rewrite state, calculator, commit/replay advancement, tests, and new
spserialization name are internally consistent. The inline comment identifies the remaining old terminology that conflicts with the PR's stated clarity goal. - Concurrency and lifecycle: no lock, shared-state, or ordering behavior changes. Local commit and COMMITTED replay still advance exactly once under existing table/transaction locking; VISIBLE replay does not advance again. Dry run and EXPLAIN remain read-only.
- Persistence and compatibility: current images and
AlterMTMVjournals consistently persistsp; checkpoint/restart/failover paths were traced. Oldrvor mixed-version IVM metadata would reset the prefix, but the repository's explicit pre-October-2026 IVM policy permits this pre-release incompatibility. - Parallel paths and encoding: local and cloud prefix ownership, linear/join/aggregate consumers, BIGINT/LARGEINT bit layouts and bounds, error paths, performance, and observability are unchanged. No config, FE-BE protocol, or additional storage-format change is introduced.
- Tests and validation: the changed unit tests retain exact encoding/boundary assertions and cover Gson/copy, normal commit, and replay. This review was static-only as required; no builds or tests were run here. At submission time, CheckStyle and compile checks were passing, while FE UT and regression jobs were still pending.
- User focus: no additional focus was provided; the complete PR was reviewed.
| ### Semantics | ||
|
|
||
| - **Read-only.** No insert transaction is built. Stream offsets, refresh version, and MV metadata | ||
| - **Read-only.** No insert transaction is built. Stream offsets, sequence prefix, and MV metadata |
Contributor
There was a problem hiding this comment.
Could you finish the terminology sweep? regression-test/plugins/plugin_planner.groovy:138 and regression-test/suites/mtmv_p0/ivm/test_ivm_refresh_dry_run.groovy:19 still say the sequence encodes a "refresh version", and the renamed transaction test still uses *_refresh_version_* fixture names. Since this refactor is specifically meant to remove that ambiguity, leaving these references keeps teaching the old concept.
morrySnow
approved these changes
Sep 21, 2026
Contributor
TPC-H: Total hot run time: 27194 ms |
Contributor
TPC-DS: Total hot run time: 152307 ms |
Contributor
ClickBench: Total hot run time: 23.85 s |
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
3 participants
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.
IvmInfo.refreshVersionis the high part of the sequence values an IVM MV's rows are stamped with -- the low part next to it is a delta index. The name reads like an epoch or like the MV's own version, which is what the per-partitionrefreshEpochbeing added alongside it is not, and the two sitting in the same code base is a trap for the next reader.This renames it to
sequencePrefix: the prefix of the(sequence prefix, delta index, op)triple thatIvmSequenceCalculatorencodes into the sequence column. Nothing about the value changes -- it still counts committing IVM transactions and still prefixes the sequence column.sequencePrefix,getSequencePrefix(),advanceSequencePrefix()MTMV.getNextRefreshVersion()->MTMV.getNextSequencePrefix()IvmSequenceCalculatoridentifiers, includingLARGEINT_SEQUENCE_PREFIX_SHIFTand the range-check messages"rv"to"sp": IVM is not released, so there is no image or journal in the wild that writes the old nameTrace: #65418