Repository navigation
[fix](table stream) min delta delete op should return first op's befo… - #65826
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29516 ms |
Contributor
TPC-DS: Total hot run time: 177298 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
previously approved these changes
Jul 21, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29781 ms |
Contributor
TPC-DS: Total hot run time: 177923 ms |
Contributor
ClickBench: Total hot run time: 24.85 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
approved these changes
Jul 22, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
yiguolei
approved these changes
Jul 23, 2026
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for three patch-scoped issues. The core history-backed fix is sound: folded DELETE payloads should use the first operation's BEFORE image while terminal TSO/LSN metadata comes from the last operation. However, the broader DETAIL change mishandles a supported compatibility layout, and the patch also removes an unrelated UPDATE_BEFORE LSN invariant.
Checkpoint conclusions:
- Goal, scope, and parallel paths: the new BE and SQL cases exercise the reported history-backed DELETE bug, but DETAIL/no-history compatibility and multi-update LSN behavior remain uncovered. The two output-only blank-line edits are non-semantic.
- Concurrency and lifecycle: materialization remains reader-local; no shared-state, locking, initialization-order, ownership, or pending-row lifetime issue was found.
- Configuration and compatibility: no new option is introduced, but schemas retaining placeholder BEFORE columns while
write_before=falseexpose the DETAIL regression noted inline. - Conditions, errors, memory, and performance: status propagation and EOF/batch-split behavior remain intact; generic column copying introduces no material allocation or hot-loop concern. All nine MIN_DELTA first/last-operation transitions were rechecked.
- Transactions, persistence, writes, and protocols: this changes read-side row materialization only; no row-binlog format, edit-log, transaction, atomicity, or external protocol change was found.
- Tests and validation: the new BE test is included by the recursive test target and the expected outputs are deterministic, but it does not assert the changed LSN behavior. Current Linux BE UT, compile, nonConcurrent/P0 regression, formatting, style, and coverage checks pass. The macOS BE UT check fails before compilation because that runner uses JDK 25 instead of the required JDK 17, so it is unrelated to this patch. Per the review-only contract, I did not run local builds or tests.
- Observability and requested focus: no logging/metrics change is needed for this local materialization fix, and no additional user-provided focus was supplied.
Two full review rounds converged: both normal reviewers and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS against this final comment set.
| @@ -381,7 +384,7 @@ Status BlockReader::_detail_change_next_block(Block* block, bool* eof) { | |||
| output_row_count++; | |||
| } else if (op == ROW_BINLOG_DELETE) { | |||
Contributor
There was a problem hiding this comment.
DETAIL is also supported for the compatibility layout where the row-binlog schema still contains __BEFORE__* columns but write_before is false. RowBinlogSegmentWriter marks that layout with _fill_empty_before_value and deliberately stores NULL placeholders in every BEFORE column. With use_before=true, _resolve_source_column_index sees that the mirror exists and emits the placeholder instead of the stored DELETE payload (for example, a full-payload delete becomes NULL). Please preserve the current column when history was not recorded, or carry enough metadata to distinguish placeholder mirrors, and add a DETAIL DELETE case for this layout.
| @@ -290,9 +296,6 @@ Status BlockReader::_min_delta_next_block(Block* block, bool* eof) { | |||
| if (idx == _binlog_op_pos) { | |||
| RETURN_IF_ERROR(_write_binlog_op(*target_columns[target_col_idx], | |||
Contributor
There was a problem hiding this comment.
This also removes the existing _binlog_lsn_pos special case, although the bug being fixed only requires payload columns to use the first BEFORE image. For updates to key 1 at LSN 100 and 102 with another key's event at 101, the folded pair now carries LSNs 100/102; ORDER BY __DORIS_BINLOG_LSN__ interleaves the unrelated event between UPDATE_BEFORE/UPDATE_AFTER. Previously both halves used the group's last LSN, consistent with the write-side max-LSN merge contract. Please restore last-row selection for LSN and add an assertion for the multi-update pair.
| FROM mow_min_delta_del_ins_del_stream | ||
| ORDER BY id, v1, __DORIS_STREAM_CHANGE_TYPE_COL__ | ||
| """ | ||
| } finally { |
Contributor
There was a problem hiding this comment.
Please do not drop the database in finally. The suite already drops/recreates it before use, and the repository test standard intentionally keeps tables after a failure so the generated stream/binlog state can be inspected. Removing this final cleanup (and the now-unneeded try/finally) preserves that debugging state for either order_qt failure.
0AyanamiRei
pushed a commit
to 0AyanamiRei/doris
that referenced
this pull request
Jul 27, 2026
wyxxxcat
pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
35 of 69 tasks
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.
…re value
What problem does this PR solve?
Issue Number: close #65800
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)