Handle Scan with Index correctly during validation logic in Consensus Commit #2808

brfrn169 · 2025-06-20T06:30:33Z

Description

This PR fixes a bug where the serializable validation fails when executing Scan with index and updating the indexed columns of the records or deleting records in the scan result.

Related issues and/or PRs

N/A

Changes made

Add some inline comments. Please take a look for the details.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

Copilot

Pull Request Overview

This PR enhances scan-with-index validation by skipping records modified by the current transaction and updates tests to cover these new cases.

Refactored preparePut/prepareDelete test helpers to use builder syntax and added overloads.
Extended Snapshot.validateScanResults to ignore own writes/deletes during scan validation.
Added new tests for index‐based scan validation and standardized “Myself” naming in existing tests.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
core/src/test/java/com/scalar/db/transaction/consensuscommit/SnapshotTest.java	Switched to builder‐based Put/Delete helpers, renamed existing tests (“MySelf” → “Myself”), and added new scan-with-index tests.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java	Updated `validateScanResults` to skip entries in `writeSet`/`deleteSet` and replaced a final null check with a loop over leftover entries.

Comments suppressed due to low confidence (1)

core/src/test/java/com/scalar/db/transaction/consensuscommit/SnapshotTest.java:1633

[nitpick] The method signature is split across two lines (public void on one line and the name on the next), which differs from existing tests. Consider collapsing this into one line for consistency and readability.

  public void

Copilot · 2025-06-20T06:32:09Z

core/src/test/java/com/scalar/db/transaction/consensuscommit/SnapshotTest.java

+        scan, Maps.newLinkedHashMap(ImmutableMap.of(key1, result1, key2, result2, key3, result3)));
+
+    // Simulate that the first and second records were updated by myself
+    snapshot.putIntoWriteSet(key1, preparePut(ANY_TEXT_1, ANY_TEXT_1));


The test claims to simulate updates by myself for the first and second records, but it only puts key1 and key3 into the writeSet. You should also add key2 to writeSet (or adjust the comment) so the test accurately reflects the intended scenario.

Suggested change

snapshot.putIntoWriteSet(key1, preparePut(ANY_TEXT_1, ANY_TEXT_1));

snapshot.putIntoWriteSet(key1, preparePut(ANY_TEXT_1, ANY_TEXT_1));

snapshot.putIntoWriteSet(key2, preparePut(ANY_TEXT_2, ANY_TEXT_1));

Copilot · 2025-06-20T06:32:10Z

core/src/test/java/com/scalar/db/transaction/consensuscommit/SnapshotTest.java

+        scan, Maps.newLinkedHashMap(ImmutableMap.of(key1, result1, key2, result2, key3, result3)));
+
+    // Simulate that the first and second records were deleted by myself
+    snapshot.putIntoDeleteSet(key1, prepareDelete(ANY_TEXT_1, ANY_TEXT_1));


The deletion test’s comment says it simulates deletes of the first and second records by myself, but it only deletes key1 and key3. Add key2 to the deleteSet (or update the comment) so the test setup matches its description.

Suggested change

snapshot.putIntoDeleteSet(key1, prepareDelete(ANY_TEXT_1, ANY_TEXT_1));

snapshot.putIntoDeleteSet(key1, prepareDelete(ANY_TEXT_1, ANY_TEXT_1));

snapshot.putIntoDeleteSet(key2, prepareDelete(ANY_TEXT_2, ANY_TEXT_1));

Copilot · 2025-06-20T06:32:10Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

+
+            // Skip the record of the original scan results
+            originalResultEntry = Iterators.getNext(originalResultIterator, null);


[nitpick] The skip logic for own writes/deletes appears twice and uses manual iterator advancement plus continue. Consider extracting this into a helper method or restructuring the loop to avoid manual getNext calls for clearer control flow.

Suggested change

// Skip the record of the original scan results

originalResultEntry = Iterators.getNext(originalResultIterator, null);

SkipResult skipResult = skipOwnWritesDeletes(latestResult, scanner, originalResultEntry, originalResultIterator, null);

originalResultEntry = skipResult.getOriginalResultEntry();

brfrn169 · 2025-06-20T06:55:21Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

+          if (writeSet.containsKey(originalResultEntry.getKey())
+              || deleteSet.containsKey(originalResultEntry.getKey())) {


We can also skip the key in writeSet or deleteSet.

brfrn169 · 2025-06-20T06:55:32Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java

+        if (writeSet.containsKey(originalResultEntry.getKey())
+            || deleteSet.containsKey(originalResultEntry.getKey())) {


Ditto. We can also skip the key in writeSet or deleteSet.

feeblefakie

LGTM! Thank you!

komamitsu

LGTM! 👍

…lidation-logic

brfrn169 · 2025-06-20T11:23:16Z

@Torch3333 I'll merge this. Please take a look when you have time!

… Commit (#2808)

Handle Scan with Index correctly during validation logic

59ae1f4

brfrn169 requested a review from Copilot June 20, 2025 06:30

brfrn169 self-assigned this Jun 20, 2025

brfrn169 added the bugfix label Jun 20, 2025

brfrn169 changed the title ~~Handle Scan with Index correctly during validation logic~~ Handle Scan with Index correctly during validation logic in Consensus Commit Jun 20, 2025

Copilot AI reviewed Jun 20, 2025

View reviewed changes

[skip ci] Fix

3b82c92

brfrn169 commented Jun 20, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie and komamitsu June 20, 2025 06:55

feeblefakie approved these changes Jun 20, 2025

View reviewed changes

komamitsu approved these changes Jun 20, 2025

View reviewed changes

Merge branch 'master' into handle-scan-with-index-correctly-during-va…

bd135af

…lidation-logic

brfrn169 merged commit 9264d5c into master Jun 20, 2025
56 checks passed

brfrn169 deleted the handle-scan-with-index-correctly-during-validation-logic branch June 20, 2025 11:23

feeblefakie pushed a commit that referenced this pull request Jun 20, 2025

Handle Scan with Index correctly during validation logic in Consensus…

f42aaf4

… Commit (#2808)

feeblefakie mentioned this pull request Jun 20, 2025

Backport to branch(3) : Handle Scan with Index correctly during validation logic in Consensus Commit #2812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle Scan with Index correctly during validation logic in Consensus Commit #2808

Handle Scan with Index correctly during validation logic in Consensus Commit #2808

Uh oh!

brfrn169 commented Jun 20, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 20, 2025

Uh oh!

Copilot AI Jun 20, 2025

Uh oh!

Copilot AI Jun 20, 2025

Uh oh!

brfrn169 Jun 20, 2025

Uh oh!

brfrn169 Jun 20, 2025

Uh oh!

feeblefakie left a comment

Uh oh!

komamitsu left a comment

Uh oh!

brfrn169 commented Jun 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	snapshot.putIntoWriteSet(key1, preparePut(ANY_TEXT_1, ANY_TEXT_1));
	snapshot.putIntoWriteSet(key1, preparePut(ANY_TEXT_1, ANY_TEXT_1));
	snapshot.putIntoWriteSet(key2, preparePut(ANY_TEXT_2, ANY_TEXT_1));

	snapshot.putIntoDeleteSet(key1, prepareDelete(ANY_TEXT_1, ANY_TEXT_1));
	snapshot.putIntoDeleteSet(key1, prepareDelete(ANY_TEXT_1, ANY_TEXT_1));
	snapshot.putIntoDeleteSet(key2, prepareDelete(ANY_TEXT_2, ANY_TEXT_1));


		// Skip the record of the original scan results
		originalResultEntry = Iterators.getNext(originalResultIterator, null);

		if (writeSet.containsKey(originalResultEntry.getKey())
		\|\| deleteSet.containsKey(originalResultEntry.getKey())) {

Handle Scan with Index correctly during validation logic in Consensus Commit #2808

Handle Scan with Index correctly during validation logic in Consensus Commit #2808

Uh oh!

Conversation

brfrn169 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

brfrn169 commented Jun 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brfrn169 commented Jun 20, 2025 •

edited

Loading