[fix](be) Suppress no-op updates in min delta scans - #66849
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: MIN_DELTA row-binlog scans previously classified a key window only from its first and last operation. A physical update therefore produced UPDATE_BEFORE and UPDATE_AFTER rows even when the complete row image did not change. Read all AFTER and BEFORE value columns for MIN_DELTA scans, compare the initial BEFORE image with the final AFTER image, and turn the result into SKIP when every comparable value is equal. Comparison-only columns remain hidden from the SQL projection, and incomplete or unsupported row images retain the existing update output conservatively.
### Release note
MIN_DELTA row-binlog queries no longer return update rows when the complete row image is unchanged across the query window.
### Check List (For Author)
- Test: Unit Test
- BlockReaderChangeNextBlockTest.* (16/16 passed under ASAN)
- Release BE and Cloud targets compiled successfully
- Behavior changed: Yes. Net-zero updates are omitted from MIN_DELTA results.
- Does this need documentation: Yes (follow-up documentation update; no doc PR yet)
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The MIN_DELTA BlockReader tests covered only short operation sequences and single-row multi-column comparisons. Add table-driven cases with repeated insert, delete, and update operations, including delete/reinsert cycles that either return to the original row image or finish changed. Also add a multi-key, multi-column case that verifies complete-row equality, hidden comparison columns, skipped groups, and the pending-row batch boundary together.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- BlockReaderChangeNextBlockTest.* (18/18 passed under ASAN)
- Behavior changed: No. Test coverage only.
- Does this need documentation: No
|
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.
Requesting changes. The endpoint-folding idea is sound for complete, comparable row images, but seven blockers remain: three false-suppression/data-correctness cases (unavailable BEFORE images, generated-name collisions, and signed zero), one production query-failure case (AGG_STATE), and three performance regressions (two independent O(C^2) setup passes and avoidable full I/O for unsupported schemas).
Critical checkpoint conclusions:
- Goal and tests: the new integer unit cases cover first/last endpoint folding, hidden unprojected values, multi-key groups, EOF, pending rows, and batch-size boundaries. They do not cover AGG_STATE concrete columns, writer-side missing/placeholder BEFORE images, generated-name collisions, unsupported schemas, wide-table initialization, signed zero, or end-to-end writer/scanner integration.
- Correctness, nullability, and compatibility: the AGG_STATE, unavailable-BEFORE, generated-name, and signed-zero issues in the inline comments are reachable on supported row-binlog paths. Legacy/compatibility placeholder images need explicit per-row validity; ordering and name equality are not sufficient row-state identity.
- Scope and lifecycle: FE schema generation, the row-binlog writer/retriever, scanner projection, BlockReader grouping, schema evolution, cloud/non-cloud paths, DETAIL/APPEND_ONLY modes, key boundaries, EOF, and pending UPDATE_AFTER rows were traced. Ordinary grouping and batch lifecycle are sound; DETAIL/APPEND_ONLY are outside the new equality path, and schema mismatches conservatively retain UPDATE.
- Concurrency, configuration, persistence, and transactions: no new shared state, locking, configuration, on-disk format, or transaction-protocol change was found. The read-side false suppressions can nevertheless leave downstream incremental state incorrect.
- Performance: the scanner projection and BlockReader name resolution are separate quadratic per-scanner setup passes; unsupported schemas also read all comparison-only values even though suppression is statically impossible.
- Coverage and CI: no tests were run in this review environment. Completed style, license, and related checks pass; BE unit-test, compile, and performance checks are still pending at review time.
- User focus: no additional focus was provided.
- Review completion: capped/incomplete. Three review rounds were executed; the signed-zero issue first surfaced in the final permitted round, so this run cannot claim convergence even though all current candidates were independently adjudicated.
| // families. Row binlog currently rejects VARIANT, while the remaining types are kept here as a | ||
| // conservative guard so an old or malformed schema cannot turn a MIN_DELTA query into an error. | ||
| bool supports_min_delta_value_comparison(PrimitiveType type) { | ||
| return !is_var_len_object(type) && type != TYPE_VARIANT && type != TYPE_FIXED_LENGTH_OBJECT && |
There was a problem hiding this comment.
[P1] Guard the concrete AGG_STATE representation before comparing. TYPE_AGG_STATE passes this allowlist, but DataTypeAggState creates aggregate-specific serialized columns: fixed states such as sum/count use ColumnFixedLengthObject (no compare_at override), while bitmap-style states use ColumnComplexType whose override is BE_TEST-only. Row-binlog schema construction permits AGG_STATE and wraps value cells nullable, so a non-NULL update reaches _min_delta_values_equal() and throws NOT_IMPLEMENTED_ERROR instead of conservatively retaining the UPDATE. Please inspect the actual serialized column capability or exclude AGG_STATE here.
| auto result = binlog::AggregateFunctionMinDelta::calculate_result(first_op, last_op); | ||
| if (result == binlog::AggregateFunctionMinDelta::ResultType::UPDATE_BEFORE_AFTER && | ||
| binlog::is_valid_row_binlog_op(first_op) && binlog::is_valid_row_binlog_op(last_op) && | ||
| _min_delta_values_equal(group_size - 1)) { |
There was a problem hiding this comment.
[P1] Do not treat an unavailable BEFORE image as a real all-NULL row. build_before_block() fills every BEFORE value with NULL when no historical row exists. A delete sign for a missing key is still recorded as DELETE, and a later reinsertion after that tombstone becomes APPEND. If all value columns are NULL, DELETE-to-APPEND first yields UPDATE_BEFORE_AFTER, then this gate sees NULL equal to NULL and drops the net absent-to-present INSERT as SKIP. Compatibility placeholder BEFORE rows have the same ambiguity. Only suppress when the first BEFORE image is known valid, or conservatively retain ambiguous all-NULL images.
| const uint32_t col_num = src_block.columns(); | ||
| _before_column_idx.resize(col_num); | ||
| std::iota(_before_column_idx.begin(), _before_column_idx.end(), 0); | ||
| std::vector<bool> is_before_value_column(col_num, false); |
There was a problem hiding this comment.
[P1] Build the name map once before resolving mirrors. MIN_DELTA now feeds every AFTER and BEFORE column into this block, but the loop below calls Block::get_position_by_name()--an O(C) scan--for each physical column. A table with V values has roughly 2V value/mirror columns, so every BlockReader performs O(C^2) string comparisons, repeated per tablet scanner. Please use Block::get_name_to_pos_map() once here and resolve every BEFORE name from it.
| // No-op UPDATE detection compares the complete row state at the two ends of the | ||
| // window. Read every AFTER/BEFORE value column even when SQL projects only a subset; | ||
| // BlockReader's return-column mapping keeps these comparison-only columns hidden. | ||
| for (uint32_t cid = tablet_schema->num_key_columns(); |
There was a problem hiding this comment.
[P1] Preflight unsupported types before widening the projection. If any value is BITMAP, HLL, QUANTILE_STATE, or another rejected shape, BlockReader permanently sets comparison completeness to false, so this optimization can never suppress a group. This loop nevertheless reads every unrequested AFTER and BEFORE value on every block; even a key-only MIN_DELTA query on a BITMAP table now pays the large extra I/O for no behavior change. When complete comparison is statically impossible, preserve the old narrow projection (and keep the runtime check conservative).
| std::string before_name = binlog::build_before_column_name(name); | ||
| int tmp_idx = src_block.get_position_by_name(before_name); | ||
| _before_column_idx[i] = tmp_idx < 0 ? i : tmp_idx; | ||
| if (tmp_idx >= 0) { |
There was a problem hiding this comment.
[P1] Pair BEFORE values by schema identity, not first matching name. Doris permits a user value named BEFORE__v, while row-binlog generation also gives v's mirror that name. Block::get_position_by_name() selects the earlier user AFTER column, so this loop builds a complete-looking but wrong graph. For two same-typed values v and BEFORE__v, updates (0,1)->(1,2)->(2,1) make all three erroneous comparisons equal and this PR emits SKIP even though v changed from 0 to 2. Please disambiguate generated names or resolve pairs by unique id/ordinal, and cover this collision end to end.
| // BlockReader's return-column mapping keeps these comparison-only columns hidden. | ||
| for (uint32_t cid = tablet_schema->num_key_columns(); | ||
| cid < tablet_schema->num_columns(); ++cid) { | ||
| add_return_column_if_absent(cid); |
There was a problem hiding this comment.
[P1] Build this full projection without a linear search per column. add_return_column_if_absent() runs std::find over the growing return_columns vector, and this new loop calls it for all C physical binlog columns, so each tablet/range scanner now does roughly C^2/2 integer comparisons even for a narrow query. That is separate from the quadratic name lookup later in BlockReader. Please track selected dense column ids with a bitmap/set or append the remaining schema columns in one linear pass while preserving the required output order.
| return false; | ||
| } | ||
| for (const auto& [after_idx, before_idx] : _min_delta_value_column_pairs) { | ||
| if (_stored_data_columns[before_idx]->compare_at( |
There was a problem hiding this comment.
[P1] Do not suppress an observable signed-zero update. FLOAT and DOUBLE pass the new gate, but CompareFloat (used by ColumnVector::compare_at) returns 0 for +0.0 versus -0.0. Doris preserves and displays both signs, and signbit(+0.0) / signbit(-0.0) returns false / true, so a valid MOW update between them changes row state. This comparison turns that update into SKIP and can leave an incremental consumer's derived state stale; nested floats inherit the same behavior. Please use state equality that preserves signed zero, or conservatively exclude floating shapes, and add an end-to-end MIN_DELTA test.
|
Codex automated review failed and did not complete. Error: Codex completed, but the workflow could not verify pull request reviews through GitHub API. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-DS: Total hot run time: 82087 ms |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
MIN_DELTA row-binlog scans previously classified each primary-key window only from its first and last operation. As a result, an update emitted
UPDATE_BEFOREandUPDATE_AFTERrows even when the complete row image was unchanged, including multi-update chains that eventually returned to their original values.This change reads all AFTER/BEFORE value columns needed by MIN_DELTA, compares the first BEFORE image with the final AFTER image, and converts a net-zero update into
SKIP. Comparison-only columns remain hidden from the SQL projection. Missing, incompatible, or unsupported BEFORE images preserve the existing UPDATE output conservatively.The tests cover direct no-op updates, changes in unprojected columns, multiple keys and columns, pending output across batch boundaries, and complex insert/delete/update chains that either return to the original image or finish changed.
Release note
MIN_DELTA row-binlog queries no longer return update rows when the complete row image is unchanged across the query window.
Check List (For Author)
Test
BlockReaderChangeNextBlockTest.*: 18/18 passed under ASAN.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)