Handle Get and Scan with conjunctions correctly in Consensus Commit #2786

brfrn169 · 2025-06-18T04:59:34Z

Description

This PR updates the code to handle Get and Scan with conjunctions correctly in Consensus Commit.

When users specify conjunctions for Get and Scan operations, the same conjunctions also need to be applied to the columns in the before images. The reason is explained in the following Javadoc—please refer to it for details:

https://github.com/scalar-labs/scalardb/pull/2786/files#diff-acc93f1366b03168017001646c7217f7dd1f3c7cbe8a41b6a9a8484dc5ef0905R482-R546

Related issues and/or PRs

N/A

Changes made

Update the code to convert the conjunctions when users specify conjunctions for Get and Scan operations.

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

Fixed a bug where records could be missed when executing Get or Scan with conjunctions in Consensus Commit.

Copilot

Pull Request Overview

This PR fixes a bug in handling Get and Scan operations with conjunctions in Consensus Commit by filtering records based on before image conditions. Key changes include updating the read and scan methods to correctly throw exceptions for uncommitted records, introducing optional filtering with conjunctions, and adding logic to convert conjunctions for before image conditions.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File	Description
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Updated read and scan handling with conjunctions and before image conditions
core/src/main/java/com/scalar/db/api/LikeExpression.java	Minor documentation and type refinements for text columns
core/src/main/java/com/scalar/db/api/ConditionBuilder.java	Added a buildLikeExpression utility method with corresponding documentation

Comments suppressed due to low confidence (2)

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java:267

[nitpick] Consider renaming the variable 'ret' to a more descriptive name such as 'filteredResult' to enhance code readability.

    Optional<TransactionResult> ret = Optional.of(result);

core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java:165

If 'result' is empty and 'get.getConjunctions()' is not empty, the subsequent call to 'result.get()' in the throw statement will cause a NoSuchElementException. Consider handling the empty result case explicitly without calling 'result.get()'.

    if (result.isPresent() || get.getConjunctions().isEmpty()) {

feeblefakie

Overall, looking good.
Left one suggestion for the comment.

feeblefakie · 2025-06-18T05:55:23Z

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

+   *   | 0             | 1              | 200    | PREPARED  | 1000          | COMMITTED      |
+   * </pre>
+   *
+   * If we scan records with the condition `column = 1000`, we will only retrieve the first record,


The description is a bit weird to me.
How about something like below?

If we scan records with the condition "column = 1000" without converting the condition (conjunction), we only get the first record, not the second one, because the condition does not match. However, the second record has not been committed yet, so we should still retrieve it, considering the possibility that the record will be rolled back.

Fixed in 683337b. Thanks!

feeblefakie

LGTM! Thank you!

komamitsu · 2025-06-18T08:11:14Z

...va/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java

+
+  @Test
+  public void
+      get_WithConjunction_ForCommittedRecordWhoseBeforeImageMatchesConjunction_ShouldNotReturnRecordAfterLazyRecovery()


IIUC, the final state is already COMMITTED and no lazy recovery occurs?

Good catch! Let me fix it.

Fixed in 6c16dcd. Thanks!

komamitsu

LGTM! 👍

jnmt

Thank you for finding the bug. Overall looks good to me. I just left a confirmation. PTAL!

jnmt · 2025-06-18T08:50:58Z

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

+      if (key != null) {
+        putIntoReadSetInSnapshot(key, result);
+      } else {
+        // Only for a Get with index, the argument `key` is null
+
+        if (result.isPresent()) {
+          // Only when we can get the record with the Get with index, we can put it into the read
+          // set
+          key = new Snapshot.Key(get, result.get());
+          putIntoReadSetInSnapshot(key, result);


Confirmation. The issue we discussed before (unexpected snapshot overwrite) will be resolved in another PR, or am I missing the point?

We will address that issue in another PR. Thanks!

Got it. Thank you!

jnmt

LGTM, thank you!

jnmt · 2025-06-18T09:35:48Z

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

-            putIntoReadSetInSnapshot(key, result);
-          }
+    if (!get.getConjunctions().isEmpty()) {
+      // Check if the result matches the conjunctions


It's likely this check exists because we get records based on not only the current image but also before image using OR conditions. We would like to filter the case where the second record is committed in the example of the Javadoc comment. If my understanding is correct, it might be nice to have a comment like this for future readers to easily grasp the reasoning.

Suggested change

// Check if the result matches the conjunctions

// Because we also get records whose before images match the conjunctions, we need to check if

// the current status of the records actually match the conjunctions.

Fixed in 4ec25d6. Thanks!

jnmt · 2025-06-18T09:36:02Z

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

+      if (key != null) {
+        putIntoReadSetInSnapshot(key, result);
+      } else {
+        // Only for a Get with index, the argument `key` is null
+
+        if (result.isPresent()) {
+          // Only when we can get the record with the Get with index, we can put it into the read
+          // set
+          key = new Snapshot.Key(get, result.get());
+          putIntoReadSetInSnapshot(key, result);


Got it. Thank you!

jnmt · 2025-06-18T09:37:07Z

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

-    putIntoReadSetInSnapshot(key, Optional.of(result));
+    Optional<TransactionResult> ret = Optional.of(result);
+    if (!scan.getConjunctions().isEmpty()) {
+      // Check if the result matches the conjunctions


Ditto.

Suggested change

// Check if the result matches the conjunctions

// Because we also get records whose before images match the conjunctions, we need to check if

// the current status of the records actually match the conjunctions.

Fixed in 4ec25d6. Thanks!

…ecctly-in-consensus-commit

brfrn169 · 2025-06-18T14:37:38Z

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

…2786)

Handle Get and Scan with conjunctions correctly in Consensus Commit

25a154f

brfrn169 requested a review from Copilot June 18, 2025 04:59

brfrn169 self-assigned this Jun 18, 2025

brfrn169 added the bugfix label Jun 18, 2025

Copilot AI reviewed Jun 18, 2025

View reviewed changes

brfrn169 requested review from Torch3333, feeblefakie, jnmt and komamitsu June 18, 2025 05:08

feeblefakie reviewed Jun 18, 2025

View reviewed changes

[skip ci] Fix Javadoc

683337b

brfrn169 requested a review from feeblefakie June 18, 2025 06:44

feeblefakie approved these changes Jun 18, 2025

View reviewed changes

komamitsu reviewed Jun 18, 2025

View reviewed changes

Fix based on feedback

6c16dcd

komamitsu approved these changes Jun 18, 2025

View reviewed changes

jnmt reviewed Jun 18, 2025

View reviewed changes

brfrn169 requested a review from jnmt June 18, 2025 08:57

jnmt approved these changes Jun 18, 2025

View reviewed changes

brfrn169 added 2 commits June 18, 2025 19:54

[skip ci] Fix based on feedback

4ec25d6

Merge branch 'master' into handle-get-and-scan-with-conjunctions-corr…

c01a3db

…ecctly-in-consensus-commit

brfrn169 merged commit aadc7f8 into master Jun 18, 2025
53 checks passed

brfrn169 deleted the handle-get-and-scan-with-conjunctions-correcctly-in-consensus-commit branch June 18, 2025 14:37

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

Handle Get and Scan with conjunctions correctly in Consensus Commit (#…

2be0e0b

…2786)

feeblefakie mentioned this pull request Jun 18, 2025

Backport to branch(3) : Handle Get and Scan with conjunctions correctly in Consensus Commit #2796

Merged

	// Check if the result matches the conjunctions
	// Because we also get records whose before images match the conjunctions, we need to check if
	// the current status of the records actually match the conjunctions.

Handle Get and Scan with conjunctions correctly in Consensus Commit #2786

Handle Get and Scan with conjunctions correctly in Consensus Commit #2786

Conversation

brfrn169 commented Jun 18, 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!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

jnmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brfrn169 commented Jun 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented Jun 18, 2025 •

edited

Loading