Handle Get with index correctly in CrudHandler

[brfrn169]

Description

Currently, we create a Snapshot.Key instance from the Get object in CrudHandler, as shown here:

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

Line 79 in 63520eb

Snapshot.Key key = new Snapshot.Key(get);

This approach works for regular Get operations. However, it is incorrect for Get operations using a secondary index, as they do not contain primary key information. In such cases, we should create the Snapshot.Key instance from the result of the secondary index Get operation. This bug can lead to unnecessary implicit pre-reads when updating records retrieved by a secondary index Get operation within a transaction.

To fix this issue, this PR updates CrudHandler to correctly handle Get operations that use secondary indexes.

Related issues and/or PRs

N/A

Changes made

I’ve added 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

Fixed a bug that caused unnecessary implicit pre-reads when updating records retrieved by a secondary index Get operation within a transaction.

[Copilot]

Pull Request Overview

This PR enhances the CrudHandler to correctly handle Get operations that use a secondary index by deferring key construction until after the storage read and by updating both handler logic and snapshot state accordingly.

• Introduces @Nullable key handling in CrudHandler.get, readUnread, and read for index-based queries.
• Adds a new Snapshot.Key(Get, Result) constructor to build the key post-read when using an index.
• Extends unit and integration tests to verify Get/Scan with index flows, including update scenarios and snapshot interactions.

Reviewed Changes

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

File	Description
integration-test/.../ConsensusCommitSpecificIntegrationTestBase.java	New integration tests for “get with index” update and scan/update flows.
core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java	Tests added for readUnread behavior on index-based Get.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java	Added a Key(Get, Result) constructor to derive keys after fetching via index.
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java	Modified get, readUnread, and read to handle nullable keys for secondary-index Gets.

Comments suppressed due to low confidence (2)

core/src/test/java/com/scalar/db/transaction/consensuscommit/CrudHandlerTest.java:59

• [nitpick] The constant ANY_NAME_3 is not self-explanatory. Rename it to something like SECONDARY_INDEX_COLUMN to clarify that it represents the indexed field in these tests.

private static final String ANY_NAME_3 = "name3";

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

• [nitpick] The Javadoc for this method should be updated to explain how secondary-index Gets are treated (e.g., key construction is deferred and passed as null). This will help future maintainers understand the dual path.

public Optional<Result> get(Get originalGet) throws CrudException {

[Copilot]

[nitpick] Add a JavaDoc comment to this constructor clarifying that it builds a snapshot key from an index-based Get and its corresponding Result, detailing which fields are used.

Suggested change

	/**
	* Constructs a snapshot key from an index-based {@link Get} operation and its corresponding {@link Result}.
	*
	* <p>The following fields are used:
	* <ul>
	* <li>{@code namespace} and {@code table} are extracted from the {@link Get} operation.</li>
	* <li>{@code partitionKey} and {@code clusteringKey} are extracted from the {@link Result}.</li>
	* </ul>
	*
	* @param get the {@link Get} operation representing the index-based query
	* @param result the {@link Result} containing the actual data for the key
	*/

[Copilot]

[nitpick] Since this is Java 9+, consider using List.of(...) instead of Arrays.asList(...) for brevity and to get an immutable list by default.

Suggested change

	Arrays.asList(
	List.of(

[brfrn169]

For Get with index, set key to null at this point.

[brfrn169]

Create a Snapshot.Key instance from the result of the Get with index.

[feeblefakie]

LGTM! Thank you!

[komamitsu]

LGTM! 👍

[Torch3333]

LGTM, thank you!