Fix testCommitTimeCompactionPreservesDeletedRecords ignoring delete record column#18473
Merged
Jackie-Jiang merged 1 commit intoMay 12, 2026
Conversation
…ecord column The test configured `deleteRecordColumn` via `updateTableConfig` after `addTableConfig` had already created the table. The upsert metadata manager captures `_deleteRecordColumn` once during construction (`final` field on `BasePartitionUpsertMetadataManager`, populated from `UpsertContext` in `BaseTableUpsertMetadataManager#init`), so a later table-config update writes ZK but does not reconfigure the in-memory manager. Soft-deleted records were never filtered out and the pre-commit logical count was 4 instead of 2. Add an overload of `createUpsertTable` that accepts an optional delete record column and applies it to the `UpsertConfig` before `addTableConfig`, and route the three table-setup calls in `testCommitTimeCompactionPreservesDeletedRecords` through it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xiangfu0
approved these changes
May 12, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18473 +/- ##
=========================================
Coverage 63.68% 63.68%
Complexity 1684 1684
=========================================
Files 3262 3262
Lines 199826 199826
Branches 31031 31031
=========================================
+ Hits 127264 127268 +4
+ Misses 62414 62408 -6
- Partials 10148 10150 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tarun11Mavani
approved these changes
May 12, 2026
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
4 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.
Summary
testCommitTimeCompactionPreservesDeletedRecordsinCommitTimeCompactionIntegrationTestconfigureddeleteRecordColumnviaupdateTableConfigafteraddTableConfighad already created each of the three tables. The pre-commit assertion at line 1126 (validatePreCommitState) expects 2 logical records but observes 4, because soft-deleted rows are never filtered out.Root cause
The upsert metadata manager captures
_deleteRecordColumnonce at construction time — it is afinalfield onBasePartitionUpsertMetadataManager(line 89), populated fromUpsertContextinBaseTableUpsertMetadataManager#init(BaseTableUpsertMetadataManager.java:125-146). A subsequentupdateTableConfigwrites the new value to ZooKeeper but does not rebuild the in-memory manager, so the consumer never recognizes thedeletedcolumn and the valid-doc bitmap is not narrowed.This regression dates back to commit
93de8f0dc7(Add support for commit time compaction with columnMajorBuild, #16769), which refactored the test from setting the delete column onUpsertConfigbeforesetUpTable(...)(the pattern still used inUpsertTableIntegrationTest) to thecreateUpsertTable(...)+ post-createupdateTableConfig(...)pattern that doesn't take effect.Fix
Add an overload of
createUpsertTablethat accepts an optionaldeleteRecordColumnand threads it onto theUpsertConfigbeforeaddTableConfig. Route the three table-setup blocks intestCommitTimeCompactionPreservesDeletedRecordsthrough the new overload and drop the now-unnecessary post-createsetDeleteRecordColumn+updateTableConfigcalls. The other tests in the file are unaffected.