Skip to content

Commit b81e87d

Browse files
brfrn169brfrn169
committed
Handle Get with index correctly in CrudHandler (#2700)
1 parent 6badbd1 commit b81e87d

File tree

4 files changed

+264
-5
lines changed

4 files changed

+264
-5
lines changed

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

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.util.List;
2929
import java.util.Optional;
3030
import java.util.stream.Collectors;
31+
import javax.annotation.Nullable;
3132
import javax.annotation.concurrent.NotThreadSafe;
3233
import org.slf4j.Logger;
3334
import org.slf4j.LoggerFactory;
@@ -76,17 +77,27 @@ public CrudHandler(
7677
public Optional<Result> get(Get originalGet) throws CrudException {
7778
List<String> originalProjections = new ArrayList<>(originalGet.getProjections());
7879
Get get = (Get) prepareStorageSelection(originalGet);
79-
Snapshot.Key key = new Snapshot.Key(get);
80-
readUnread(key, get);
8180

8281
TableMetadata metadata = getTableMetadata(get);
82+
83+
Snapshot.Key key;
84+
if (ScalarDbUtils.isSecondaryIndexSpecified(get, metadata)) {
85+
// In case of a Get with index, we don't know the key until we read the record
86+
key = null;
87+
} else {
88+
key = new Snapshot.Key(get);
89+
}
90+
91+
readUnread(key, get);
92+
8393
return snapshot
8494
.getResult(key, get)
8595
.map(r -> new FilteredResult(r, originalProjections, metadata, isIncludeMetadataEnabled));
8696
}
8797

98+
// Only for a Get with index, the argument `key` is null
8899
@VisibleForTesting
89-
void readUnread(Snapshot.Key key, Get get) throws CrudException {
100+
void readUnread(@Nullable Snapshot.Key key, Get get) throws CrudException {
90101
if (!snapshot.containsKeyInGetSet(get)) {
91102
read(key, get);
92103
}
@@ -95,7 +106,7 @@ void readUnread(Snapshot.Key key, Get get) throws CrudException {
95106
// Although this class is not thread-safe, this method is actually thread-safe, so we call it
96107
// concurrently in the implicit pre-read
97108
@VisibleForTesting
98-
void read(Snapshot.Key key, Get get) throws CrudException {
109+
void read(@Nullable Snapshot.Key key, Get get) throws CrudException {
99110
Optional<TransactionResult> result = getFromStorage(get);
100111
if (!result.isPresent() || result.get().isCommitted()) {
101112
if (result.isPresent() || get.getConjunctions().isEmpty()) {
@@ -104,7 +115,18 @@ void read(Snapshot.Key key, Get get) throws CrudException {
104115
// transaction read it first. However, we update it only if a get operation has no
105116
// conjunction or the result exists. This is because we don’t know whether the record
106117
// actually exists or not due to the conjunction.
107-
snapshot.putIntoReadSet(key, result);
118+
if (key != null) {
119+
snapshot.putIntoReadSet(key, result);
120+
} else {
121+
// Only for a Get with index, the argument `key` is null
122+
123+
if (result.isPresent()) {
124+
// Only when we can get the record with the Get with index, we can put it into the read
125+
// set
126+
key = new Snapshot.Key(get, result.get());
127+
snapshot.putIntoReadSet(key, result);
128+
}
129+
}
108130
}
109131
snapshot.putIntoGetSet(get, result); // for re-read and validation
110132
return;

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -720,6 +720,13 @@ public Key(Get get) {
720720
this((Operation) get);
721721
}
722722

723+
public Key(Get get, Result result) {
724+
this.namespace = get.forNamespace().get();
725+
this.table = get.forTable().get();
726+
this.partitionKey = result.getPartitionKey().get();
727+
this.clusteringKey = result.getClusteringKey();
728+
}
729+
723730
public Key(Put put) {
724731
this((Operation) put);
725732
}

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ public class CrudHandlerTest {
5656
private static final String ANY_ID_2 = "id2";
5757
private static final String ANY_NAME_1 = "name1";
5858
private static final String ANY_NAME_2 = "name2";
59+
private static final String ANY_NAME_3 = "name3";
5960
private static final String ANY_TEXT_1 = "text1";
6061
private static final String ANY_TEXT_2 = "text2";
6162
private static final String ANY_TEXT_3 = "text3";
@@ -66,8 +67,10 @@ public class CrudHandlerTest {
6667
TableMetadata.newBuilder()
6768
.addColumn(ANY_NAME_1, DataType.TEXT)
6869
.addColumn(ANY_NAME_2, DataType.TEXT)
70+
.addColumn(ANY_NAME_3, DataType.TEXT)
6971
.addPartitionKey(ANY_NAME_1)
7072
.addClusteringKey(ANY_NAME_2)
73+
.addSecondaryIndex(ANY_NAME_3)
7174
.build());
7275
private static final TransactionTableMetadata TRANSACTION_TABLE_METADATA =
7376
new TransactionTableMetadata(TABLE_METADATA);
@@ -928,6 +931,7 @@ public void readUnread_GetContainedInGetSet_ShouldCallAppropriateMethods()
928931

929932
// Assert
930933
verify(storage, never()).get(any());
934+
verify(snapshot, never()).putIntoReadSet(any(Snapshot.Key.class), any(Optional.class));
931935
verify(snapshot, never()).putIntoGetSet(any(Get.class), any(Optional.class));
932936
}
933937

@@ -1014,6 +1018,7 @@ public void readUnread_GetContainedInGetSet_ShouldCallAppropriateMethods()
10141018
// Assert
10151019
verify(storage).get(any());
10161020
verify(snapshot).putIntoReadSet(key, Optional.of(new TransactionResult(result)));
1021+
verify(snapshot).putIntoGetSet(getForKey, Optional.of(new TransactionResult(result)));
10171022
}
10181023

10191024
@Test
@@ -1050,6 +1055,88 @@ public void readUnread_GetContainedInGetSet_ShouldCallAppropriateMethods()
10501055
});
10511056
}
10521057

1058+
@Test
1059+
public void
1060+
readUnread_NullKeyAndGetWithIndexNotContainedInGetSet_EmptyResultReturnedByStorage_ShouldCallAppropriateMethods()
1061+
throws CrudException, ExecutionException {
1062+
// Arrange
1063+
Get getWithIndex =
1064+
Get.newBuilder()
1065+
.namespace(ANY_NAMESPACE_NAME)
1066+
.table(ANY_TABLE_NAME)
1067+
.indexKey(Key.ofText(ANY_NAME_3, ANY_TEXT_1))
1068+
.build();
1069+
when(snapshot.containsKeyInGetSet(getWithIndex)).thenReturn(false);
1070+
when(storage.get(any())).thenReturn(Optional.empty());
1071+
1072+
// Act
1073+
handler.readUnread(null, getWithIndex);
1074+
1075+
// Assert
1076+
verify(storage).get(any());
1077+
verify(snapshot, never()).putIntoReadSet(any(), any());
1078+
verify(snapshot).putIntoGetSet(getWithIndex, Optional.empty());
1079+
}
1080+
1081+
@Test
1082+
public void
1083+
readUnread_NullKeyAndGetWithIndexNotContainedInGetSet_CommittedRecordReturnedByStorage_ShouldCallAppropriateMethods()
1084+
throws CrudException, ExecutionException {
1085+
// Arrange
1086+
Result result = mock(Result.class);
1087+
when(result.getInt(Attribute.STATE)).thenReturn(TransactionState.COMMITTED.get());
1088+
when(result.getPartitionKey()).thenReturn(Optional.of(Key.ofText(ANY_NAME_1, ANY_TEXT_1)));
1089+
when(result.getClusteringKey()).thenReturn(Optional.of(Key.ofText(ANY_NAME_2, ANY_TEXT_2)));
1090+
when(storage.get(any())).thenReturn(Optional.of(result));
1091+
1092+
Get getWithIndex =
1093+
Get.newBuilder()
1094+
.namespace(ANY_NAMESPACE_NAME)
1095+
.table(ANY_TABLE_NAME)
1096+
.indexKey(Key.ofText(ANY_NAME_3, ANY_TEXT_1))
1097+
.build();
1098+
when(snapshot.containsKeyInGetSet(getWithIndex)).thenReturn(false);
1099+
1100+
// Act
1101+
handler.readUnread(null, getWithIndex);
1102+
1103+
// Assert
1104+
verify(storage).get(any());
1105+
verify(snapshot)
1106+
.putIntoReadSet(
1107+
new Snapshot.Key(getWithIndex, result), Optional.of(new TransactionResult(result)));
1108+
verify(snapshot).putIntoGetSet(getWithIndex, Optional.of(new TransactionResult(result)));
1109+
}
1110+
1111+
@Test
1112+
public void
1113+
readUnread_NullKeyAndGetWithIndexNotContainedInGetSet_UncommittedRecordReturnedByStorage_ShouldThrowUncommittedRecordException()
1114+
throws ExecutionException {
1115+
// Arrange
1116+
Result result = mock(Result.class);
1117+
when(result.getInt(Attribute.STATE)).thenReturn(TransactionState.PREPARED.get());
1118+
when(storage.get(any())).thenReturn(Optional.of(result));
1119+
1120+
Get getWithIndex =
1121+
Get.newBuilder()
1122+
.namespace(ANY_NAMESPACE_NAME)
1123+
.table(ANY_TABLE_NAME)
1124+
.indexKey(Key.ofText(ANY_NAME_3, ANY_TEXT_1))
1125+
.build();
1126+
when(snapshot.containsKeyInGetSet(getWithIndex)).thenReturn(false);
1127+
1128+
// Act Assert
1129+
assertThatThrownBy(() -> handler.readUnread(null, getWithIndex))
1130+
.isInstanceOf(UncommittedRecordException.class)
1131+
.satisfies(
1132+
e -> {
1133+
UncommittedRecordException exception = (UncommittedRecordException) e;
1134+
assertThat(exception.getSelection()).isEqualTo(getWithIndex);
1135+
assertThat(exception.getResults().size()).isEqualTo(1);
1136+
assertThat(exception.getResults().get(0)).isEqualTo(result);
1137+
});
1138+
}
1139+
10531140
@Test
10541141
public void readIfImplicitPreReadEnabled_ShouldCallAppropriateMethods() throws CrudException {
10551142
// Arrange

‎integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4531,6 +4531,149 @@ public void get_GetWithIndexGiven_WithSerializable_ShouldNotThrowAnyException()
45314531
assertThatThrownBy(transaction::commit).isInstanceOf(CommitConflictException.class);
45324532
}
45334533

4534+
@Test
4535+
public void getAndUpdate_GetWithIndexGiven_ShouldUpdate() throws TransactionException {
4536+
// Arrange
4537+
manager.insert(
4538+
Insert.newBuilder()
4539+
.namespace(namespace1)
4540+
.table(TABLE_1)
4541+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4542+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4543+
.intValue(BALANCE, INITIAL_BALANCE)
4544+
.build());
4545+
4546+
// Act Assert
4547+
DistributedTransaction transaction = manager.begin();
4548+
Optional<Result> actual =
4549+
transaction.get(
4550+
Get.newBuilder()
4551+
.namespace(namespace1)
4552+
.table(TABLE_1)
4553+
.indexKey(Key.ofInt(BALANCE, INITIAL_BALANCE))
4554+
.build());
4555+
4556+
assertThat(actual).isPresent();
4557+
assertThat(actual.get().getInt(ACCOUNT_ID)).isEqualTo(0);
4558+
assertThat(actual.get().getInt(ACCOUNT_TYPE)).isEqualTo(0);
4559+
assertThat(actual.get().getInt(BALANCE)).isEqualTo(INITIAL_BALANCE);
4560+
4561+
transaction.update(
4562+
Update.newBuilder()
4563+
.namespace(namespace1)
4564+
.table(TABLE_1)
4565+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4566+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4567+
.intValue(BALANCE, 1)
4568+
.build());
4569+
4570+
transaction.commit();
4571+
4572+
transaction = manager.begin();
4573+
actual =
4574+
transaction.get(
4575+
Get.newBuilder()
4576+
.namespace(namespace1)
4577+
.table(TABLE_1)
4578+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4579+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4580+
.build());
4581+
transaction.commit();
4582+
4583+
assertThat(actual).isPresent();
4584+
assertThat(actual.get().getInt(ACCOUNT_ID)).isEqualTo(0);
4585+
assertThat(actual.get().getInt(ACCOUNT_TYPE)).isEqualTo(0);
4586+
assertThat(actual.get().getInt(BALANCE)).isEqualTo(1);
4587+
}
4588+
4589+
@Test
4590+
public void scanAndUpdate_ScanWithIndexGiven_ShouldUpdate() throws TransactionException {
4591+
// Arrange
4592+
manager.mutate(
4593+
Arrays.asList(
4594+
Insert.newBuilder()
4595+
.namespace(namespace1)
4596+
.table(TABLE_1)
4597+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4598+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4599+
.intValue(BALANCE, INITIAL_BALANCE)
4600+
.build(),
4601+
Insert.newBuilder()
4602+
.namespace(namespace1)
4603+
.table(TABLE_1)
4604+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4605+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 1))
4606+
.intValue(BALANCE, INITIAL_BALANCE)
4607+
.build()));
4608+
4609+
// Act Assert
4610+
DistributedTransaction transaction = manager.begin();
4611+
List<Result> actualResults =
4612+
transaction.scan(
4613+
Scan.newBuilder()
4614+
.namespace(namespace1)
4615+
.table(TABLE_1)
4616+
.indexKey(Key.ofInt(BALANCE, INITIAL_BALANCE))
4617+
.build());
4618+
4619+
assertThat(actualResults).hasSize(2);
4620+
Set<Integer> expectedTypes = Sets.newHashSet(0, 1);
4621+
for (Result result : actualResults) {
4622+
assertThat(result.getInt(ACCOUNT_ID)).isEqualTo(0);
4623+
expectedTypes.remove(result.getInt(ACCOUNT_TYPE));
4624+
assertThat(result.getInt(BALANCE)).isEqualTo(INITIAL_BALANCE);
4625+
}
4626+
assertThat(expectedTypes).isEmpty();
4627+
4628+
transaction.update(
4629+
Update.newBuilder()
4630+
.namespace(namespace1)
4631+
.table(TABLE_1)
4632+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4633+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4634+
.intValue(BALANCE, 1)
4635+
.build());
4636+
transaction.update(
4637+
Update.newBuilder()
4638+
.namespace(namespace1)
4639+
.table(TABLE_1)
4640+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4641+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 1))
4642+
.intValue(BALANCE, 2)
4643+
.build());
4644+
4645+
transaction.commit();
4646+
4647+
transaction = manager.begin();
4648+
Optional<Result> actual1 =
4649+
transaction.get(
4650+
Get.newBuilder()
4651+
.namespace(namespace1)
4652+
.table(TABLE_1)
4653+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4654+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 0))
4655+
.build());
4656+
Optional<Result> actual2 =
4657+
transaction.get(
4658+
Get.newBuilder()
4659+
.namespace(namespace1)
4660+
.table(TABLE_1)
4661+
.partitionKey(Key.ofInt(ACCOUNT_ID, 0))
4662+
.clusteringKey(Key.ofInt(ACCOUNT_TYPE, 1))
4663+
.build());
4664+
transaction.commit();
4665+
4666+
assertThat(actual1).isPresent();
4667+
assertThat(actual1.get().getInt(ACCOUNT_ID)).isEqualTo(0);
4668+
assertThat(actual1.get().getInt(ACCOUNT_TYPE)).isEqualTo(0);
4669+
assertThat(actual1.get().getInt(BALANCE)).isEqualTo(1);
4670+
4671+
assertThat(actual2).isPresent();
4672+
assertThat(actual2.get().getInt(ACCOUNT_ID)).isEqualTo(0);
4673+
assertThat(actual2.get().getInt(ACCOUNT_TYPE)).isEqualTo(1);
4674+
assertThat(actual2.get().getInt(BALANCE)).isEqualTo(2);
4675+
}
4676+
45344677
private DistributedTransaction prepareTransfer(
45354678
int fromId,
45364679
String fromNamespace,

0 commit comments

Comments
 (0)