Skip to content

[core] Restart aggregation after a removing delete - #10952

Open
cxzl25 wants to merge 1 commit into
apache:masterfrom
cxzl25:aggregation-remove-record-on-delete-reset
Open

cxzl25 wants to merge 1 commit into
apache:masterfrom
cxzl25:aggregation-remove-record-on-delete-reset

Conversation

@cxzl25

@cxzl25 cxzl25 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

With aggregation.remove-record-on-delete enabled, a delete followed by new records for the same key kept aggregating on top of the deleted record's values, because the accumulator row was replaced by the delete's own image and the field aggregators were not reset. Reset the accumulator row and every field aggregator when the first record after such a delete arrives.

Tests

Add UT

With aggregation.remove-record-on-delete enabled, a delete followed by new
records for the same key kept aggregating on top of the deleted record's
values, because the accumulator row was replaced by the delete's own image
and the field aggregators were not reset. Reset the accumulator row and
every field aggregator when the first record after such a delete arrives.

@Akash3121 Akash3121 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requirement fit: SUPPORTED. Implementation: FINDINGS.

Restarting aggregation after a removing delete fixes a real CDC scenario. Normal JDK 8 reactor verification passes all 7 targeted aggregate/function and table tests. An additional real-table checkpoint-grouping case below still retains the deleted values; the same configuration with DELETE committed separately passes. Temporary reviewer changes were removed.

Current upstream CI also has a JDK 11 core failure in SortBufferWriteBufferTestBase$WithLookupFunctionTest.testCreateReader (expected sequence 6, actual 3). I have not attributed that separate failure to this diff.

if (currentDeleteRow) {
// The record was removed by the previous delete, so later records start a new
// aggregation instead of accumulating onto the deleted record's values.
currentDeleteRow = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the delete barrier through buffer preaggregation

Resetting this accumulator is insufficient when DELETE and the first replacement INSERT are in the same checkpoint. MergeTreeWriter.flushWriteBuffer invokes this function before persisting that buffer, so getResult() emits an ordinary INSERT and loses the fact that older aggregates must be discarded. I reproduced this by adapting the new table test with supported write-only=true: commit (pt=1,a=1,b=10,c=1), then buffer its DELETE and (1,1,3,7) together before the next prepareCommit, then commit (1,1,4,8). The actual table read returns +I[1,1,17,1] instead of +I[1,1,7,7]; putting DELETE in its own commit passes the identical control. Thus the claimed delete/reinsert fix still depends on checkpoint grouping, and old sum/first_value values survive. Please preserve reset/tombstone semantics through buffer reduction and partial compaction, or avoid collapsing this boundary until older versions participate, and add a same-checkpoint persistence regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants