Add relational scan for JDBC storage and consensus commit transaction #900

jnmt · 2023-06-07T08:36:36Z

This PR adds the relational scan feature for JDBC storage. For transactions, it only supports the consensus-commit transaction. JDBC and gRPC transactions will be introduced by other PRs.

jnmt · 2023-06-07T08:57:12Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java

+      if (ScalarDbUtils.isRelational(scan)) {
+        selectQuery = buildSelectQueryForRelationalScan(scanAll, tableMetadata);


One of the major changes is starting from here. Since we decide to use ScanAll for relational scan, we do not have to change the interface of DistributedStorage, but just have to change query building for conditions and orderings.

jnmt · 2023-06-07T09:10:27Z

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

+    if (ScalarDbUtils.isRelational(scan)) {
+      // We verify if this scan does not overlap previous writes using the results obtained from
+      // the actual scan since the condition (i.e., where clause) is arbitrary in the relational
+      // scan, and thus, the write command may not have columns used in the condition, which are
+      // necessary to determine overlaps.
+      snapshot.verify(scan);
+    }


The other major change is starting from here. This prevents scan-after-write with a bit different approach (i.e., use actual scan results) in existing scans since we need to handle the case that the user's Put does not have columns required to check if they match the Scan conditions.

brfrn169

Left several comments. Please take a look when you have time!

brfrn169 · 2023-06-12T04:29:51Z

core/src/main/java/com/scalar/db/storage/cassandra/Cassandra.java

@@ -63,6 +64,21 @@ public Cassandra(DatabaseConfig config) {
    operationChecker = new OperationChecker(metadataManager);
  }

+  Cassandra(


It looks like it's only for testing. So we can add @VisibleForTesting:

Suggested change

Cassandra(

@VisibleForTesting

Cassandra(

We can do the same thing for other storage implementations.

Thank you for the suggestion. Fixed in 31d2d2b.

brfrn169 · 2023-06-12T06:02:48Z

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

+    if (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan)) {
+      // Verify overlaps only for non-relational scan since we do it later for relational scan
+      // (right before returning the results).
      throw new IllegalArgumentException("reading already written data is not allowed");
    }


I think we can move this overlap check logic into the newly added verify() method for consistency and readability. This would execute this check logic after the actual read, but I don't think this is a problem. What do you think?

Let me confirm a bit. Do you mean that we can do the overlap check after the actual scan for both cases, regardless of whether it is relational or not? If yes, the answer is yes; we can do it without losing correctness. My only concern was unnecessary additional latency, but I think the readability would have more benefits here if we assume skilled users usually know it is prohibited and can avoid it.

Do you mean that we can do the overlap check after the actual scan for both cases, regardless of whether it is relational or not?

Yes, correct.

My only concern was unnecessary additional latency, but I think the readability would have more benefits here if we assume skilled users usually know it is prohibited and can avoid it.

The IllegalArgumentException issue is basically a bug, so maybe I don't think we need to worry about the additional latency. So in this case, I think we can prioritize the readability.

OK, I will revise it. Thanks for the clarification and advice.

Fixed in 31d2d2b.

brfrn169 · 2023-06-12T06:28:52Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java

-        scan instanceof ScanAll
-            ? buildSelectQueryForScanAll((ScanAll) scan, tableMetadata)
-            : buildSelectQueryForScan(scan, tableMetadata);
+    SelectQuery selectQuery;


I think we need the same change for the scan() method to support the relational scan for JDBC transactions. Do you plan to do this in another PR?

Yes, I will add it in another PR, including tests for it.

core/src/main/java/com/scalar/db/common/checker/OperationChecker.java

komamitsu · 2023-06-13T09:09:43Z

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

-      sql += " WHERE " + conditionSqlString();
+    if (isRelationalQuery) {
+      // for relational abstraction
+      sql += relationalConditionSqlString();


How about using StringBuilder instead to avoid instantiating temporary objects? Using StringBuilder is basically better in terms of performance and memory footprint https://dev.to/this-is-learning/performance-benchmarking-string-and-string-builder-3bid

Fixed in 1490ed6.

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

komamitsu · 2023-06-13T09:31:55Z

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

+      return "";
+    }
+
+    List<String> conjunctionList = new ArrayList<>();


[minor] This can be replaced with the following code

List<String> conjunctionList = conjunctions.stream().map(conjunction -> conjunction.getConditions().stream().map(condition -> rdbEngine.enclose(condition.getColumn().getName()) + convert(condition.getOperator()) ).collect(Collectors.joining(" AND ")) ).collect(Collectors.toList());

I'm not sure which is better from the view points of readability and performance, though...

Cool, thank you. I don't think your idea loose readability very much. So, let's do it.

Fixed in 1490ed6.

komamitsu · 2023-06-13T10:26:43Z

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

+      case IS_NOT_NULL:
+        return " IS NOT NULL";
+      default:
+        throw new IllegalArgumentException("unknown operator");


How about showing the unexpected operator in the error message? I don't think it would be useful for now but for the future when we add another operator.

Fixed in 1490ed6.

komamitsu · 2023-06-13T10:30:04Z

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

@@ -127,6 +186,17 @@ private String orderBySqlString() {
            .collect(Collectors.joining(","));
  }

+  private String relationalOrderBySqlString() {
+    if (orderings.size() == 0) {


[minor] I think if (orderings.isEmpty()) would be slightly clearer.

Fixed in 1490ed6.

komamitsu · 2023-06-13T12:55:47Z

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

+        continue;
+      }
+
+      if (scan.getConjunctions().size() == 0) {


[minor] Same as above (isEmpty()) would be slightly clearer)

Fixed in 1490ed6.

komamitsu · 2023-06-13T12:57:10Z

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

+      case LTE:
+        return column.compareTo((Column<T>) condition.getColumn()) <= 0;
+      default:
+        throw new IllegalArgumentException("unknown operator");


It would be great if the unexpected operator is included in this error message (for future developments)

Fixed in 1490ed6.

komamitsu · 2023-06-13T13:44:11Z

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

+        return true;
+      }
+
+      // the following check prevents scan-after-write even if the update record potentially matches


Could you elaborate this? It would be great if you give me an example problematic case.

We need to consider three cases here to prevent scan-after-write.

A put operation overlaps the scan range regardless of the update (put) results.

A put operation does not overlap the scan range as a result of the update.

A put operation overlaps the scan range as a result of the update.

We can check cases 1 and 2 with L. 282-284. (Note that case 2 does not have the overlap, but we intentionally prohibit it due to the consistency and simplicity of snapshot management.) For case 3, we need to evaluate the operator to check for overlap since the put operation is only in the read-write set and not committed to the database. Let's see the following examples.

-- Assume the following table. -- tbl (key int, val int) -- 1 | 2 -- 2 | 3 -- Case 2 update tbl set val = 4 where key = 1; select * from tbl where val < 3; -- We can find the overlap using the scan results. -- Case 3 update tbl set val = 2 where key = 2; select * from tbl where val < 3; -- We cannot find the overlap since the database is not updated yet.

Thanks a lot for the explanation!!

@jnmt This is just a suggestion and doesn't prevent this PR from being merged. The above explanation was really helpful for me to understand why we need to consider updated values. How about putting it as a comment so that new developers and 2y later me who has a bad memory can maintain the code?

Yeah, I guess I will also forget it. It's a bit too much, but I added the detailed comments in 4b2fbe5.

Torch3333

It looks good thanks, I just have a minor comment.

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

komamitsu

LGTM! 👍

jnmt · 2023-06-14T05:20:58Z

@brfrn169 @komamitsu @Torch3333 Thank you for your review. I fixed some issues based on your comment. PTAL when you have time.

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

jnmt · 2023-06-15T03:43:26Z

SQLite integration tests sometimes failed because I didn't add ordering, and there was no guarantee to preserve the expected order. I fixed it in 7163241 and b9ea15e. Also, I noticed there is a bug in the builder part. I will fix it in another PR.

Torch3333

LGTM, thank you!

brfrn169

Left one comment. Other than that, LGTM! Thank you!

brfrn169 · 2023-06-15T09:33:02Z

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

+    if (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan)) {
+      // Verify overlaps only for non-relational scan since we do it later for relational scan
+      // (right before returning the results).
      throw new IllegalArgumentException("reading already written data is not allowed");
    }


Since it's moved to verify(), we can delete this, right?

Sorry for that. I removed it (and fix some related tests) in 2bdbae4.

brfrn169 · 2023-06-16T00:49:28Z

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

+    if ((ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWithRelational(scan))
+        || (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan))) {


Sorry, one more point. To avoid duplicate calculations, I think we should do the following:

Suggested change

if ((ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWithRelational(scan))

|| (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan))) {

boolean isRelational = ScalarDbUtils.isRelational(scan);

if ((isRelational && isWriteSetOverlappedWithRelational(scan))

|| (!isRelational && isWriteSetOverlappedWith(scan))) {

Right, I will fix it. Thank you!

@brfrn169 Fixed in 0c90275. PTAL if you have time.

feeblefakie

Very sorry for the late review.
I left several questions for clarification. PTAL!

feeblefakie · 2023-06-18T23:25:54Z

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java

+                        .collect(Collectors.joining(" AND ")))
+            .collect(Collectors.toList());
+
+    return " WHERE " + String.join(" OR ", conjunctionList);


I think we create the where clause with CNF, but it seems to be creating it with DNF.
Is it OK?
I might miss something, so let me confirm.

We are using DNF for the internal representation and building queries. So, it should be fine here.

For clarification, I added some comments for the Scan class in 19f4ada.

feeblefakie · 2023-06-18T23:53:06Z

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

+    List<Result> results = scanInternal(scan);
+
+    // We verify if this scan does not overlap previous writes after the actual scan. For a
+    // relational scan, this must be done here, using the obtained results. This is because the


It says the verify uses the obtained results, but does it?
I don't see the code that uses it.
Or you meant scanSet is updated by scanInternal and is used in the check logic (isWriteSetOverlappedWithRelational)?
I might miss something, but let me clarify this just in case.

Or you meant scanSet is updated by scanInternal and is used in the check logic (isWriteSetOverlappedWithRelational)?

Right, we use the obtained results put in the scanSet by scanInternal.

"using the obtained results" was a bit confusing since the check only uses keys in the scan set (not returned results above). I changed the comment a little in 19f4ada.

feeblefakie

LGTM! Thank you!

…#900)

Add relational scan for JDBC storage and consensus commit transaction

be6b74c

jnmt added the enhancement New feature or request label Jun 7, 2023

jnmt self-assigned this Jun 7, 2023

jnmt commented Jun 7, 2023

View reviewed changes

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 June 7, 2023 09:11

brfrn169 reviewed Jun 12, 2023

View reviewed changes

komamitsu reviewed Jun 13, 2023

View reviewed changes

core/src/main/java/com/scalar/db/common/checker/OperationChecker.java Show resolved Hide resolved

komamitsu reviewed Jun 13, 2023

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java Show resolved Hide resolved

Fix based on comments

31d2d2b

komamitsu reviewed Jun 13, 2023

View reviewed changes

Torch3333 reviewed Jun 14, 2023

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java Outdated Show resolved Hide resolved

Fix based on feedback

1490ed6

komamitsu approved these changes Jun 14, 2023

View reviewed changes

jnmt requested review from brfrn169 and Torch3333 June 14, 2023 05:21

Torch3333 reviewed Jun 14, 2023

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/query/SimpleSelectQuery.java Outdated Show resolved Hide resolved

jnmt added 3 commits June 15, 2023 10:47

Fix based on feedback

4b2fbe5

Add ordering for stable integration tests

7163241

Fix ordering again

b9ea15e

jnmt requested a review from Torch3333 June 15, 2023 03:43

Torch3333 approved these changes Jun 15, 2023

View reviewed changes

brfrn169 approved these changes Jun 15, 2023

View reviewed changes

Remove unnecessary check

2bdbae4

brfrn169 reviewed Jun 16, 2023

View reviewed changes

Fix to avoid duplicate checks

0c90275

feeblefakie reviewed Jun 19, 2023

View reviewed changes

Fix comments

19f4ada

jnmt requested a review from feeblefakie June 19, 2023 03:37

Merge branch 'master' into add-relational-scan-for-consensus-commit-tx

6ea357a

feeblefakie approved these changes Jun 20, 2023

View reviewed changes

feeblefakie merged commit eca9078 into master Jun 20, 2023
12 checks passed

feeblefakie deleted the add-relational-scan-for-consensus-commit-tx branch June 20, 2023 05:24

brfrn169 pushed a commit that referenced this pull request Jul 7, 2023

Add relational scan for JDBC storage and consensus commit transaction (…

656165b

…#900)

jnmt mentioned this pull request Nov 16, 2023

Add cross-partition scan options #1294

Merged

6 tasks

brfrn169 mentioned this pull request Nov 28, 2023

Rename Relational Scan to Cross-partition Scan #1329

Merged

6 tasks

jnmt mentioned this pull request May 14, 2024

Fix to prevent scan with index after overlapping put #1727

Merged

6 tasks

		if (ScalarDbUtils.isRelational(scan)) {
		selectQuery = buildSelectQueryForRelationalScan(scanAll, tableMetadata);

		if ((ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWithRelational(scan))
		\|\| (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan))) {

-    if ((ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWithRelational(scan))
-        || (!ScalarDbUtils.isRelational(scan) && isWriteSetOverlappedWith(scan))) {
+    boolean isRelational = ScalarDbUtils.isRelational(scan);
+    if ((isRelational && isWriteSetOverlappedWithRelational(scan))
+        || (!isRelational && isWriteSetOverlappedWith(scan))) {

Add relational scan for JDBC storage and consensus commit transaction #900

Add relational scan for JDBC storage and consensus commit transaction #900

Conversation

jnmt commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

jnmt commented Jun 14, 2023

jnmt commented Jun 15, 2023

Torch3333 left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Jun 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

brfrn169 Jun 16, 2023 •

edited