Add arbitrary filtering for get operations #1834

jnmt · 2024-06-04T07:14:28Z

Description

This PR adds arbitrary filtering for get operations.

Related issues and/or PRs

Changes made

Support the where() clause in GetBuilder, as the same as ScanBuilder.
Revise the query builder for JDBC databases to push the where clause down. (Do nothing for NoSQL databases except for adding some tests since we already introduced get operations with dynamic filtering in Support dynamic arbitrary filtering on NoSQL databases #1682)
Change Snapshot to manage the issued Get operations so that it can re-read with the where clause in the extra read.
Fix a warning message, although it’s not directly related to this PR.

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes. Will be updated in another PR.
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)

See inline comments.

Release notes

Added support for arbitrary filtering for get operations.

jnmt · 2024-06-04T08:04:43Z

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

+    DistributedTransaction transaction =
+        manager.begin(Isolation.SERIALIZABLE, SerializableStrategy.EXTRA_READ);


I first created a bug regarding the extra read with get operations since I missed that integration tests are done with snapshot isolation by default, and this setting should be added in some cases.

Note that we might also be better to use SERIALIZABLE for some other tests (e.g., get_GetGivenForCommittedRecord_ShouldReturnRecord) because they are almost the same as the one in DistributedTransactionIntegrationTestBase if we run them with SNAPSHOT. However, I left it as is for now since it's a bit out of the scope of this PR.

jnmt · 2024-06-04T08:05:07Z

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

@@ -51,6 +51,7 @@ public class Snapshot {
  private final TransactionTableMetadataManager tableMetadataManager;
  private final ParallelExecutor parallelExecutor;
  private final ConcurrentMap<Key, Optional<TransactionResult>> readSet;
+  private final Map<Get, Key> getSet;


Because we need to add the where clause into the Get operation for the extra read, as below, we start managing the issued Get operations similar to Scan.

jnmt · 2024-06-04T08:05:31Z

core/src/main/java/com/scalar/db/api/ScanBuilder.java

@@ -1582,83 +1580,4 @@ public BuildableScanFromExistingWithOngoingWhereAnd and(OrConditionSet orConditi
      return this;
    }
  }
-
-  private static class Where {


Moved to SelectionBuilder to commonly use in both GetBuilder and ScanBuilder.

jnmt · 2024-06-04T08:05:35Z

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

@@ -197,7 +197,7 @@ DistributedTransaction begin(String txId, Isolation isolation, SerializableStrat
    if (!config.getIsolation().equals(isolation)
        || !config.getSerializableStrategy().equals(strategy)) {
      logger.warn(
-          "Setting different isolation level or serializable strategy from the ones"
+          "Setting different isolation level or serializable strategy from the ones "


Since it's very minor, I fixed it in this PR.

Torch3333

LGTM, thank you.

komamitsu · 2024-06-06T05:28:45Z

core/src/main/java/com/scalar/db/api/GetBuilder.java

+
+  public static class BuildableGetWithWhere extends BuildableGet {
+
+    protected final SelectionBuilder.Where where;


[minor] This exposes the package-private Where to classes that inherit this class. How about making this to package-private?

Suggested change

protected final SelectionBuilder.Where where;

final SelectionBuilder.Where where;

Good catch, thank you! Fixed in 527d0b6.

BTW, I noticed that I can receive such warnings in IntelliJ by using a recent version. I'm using a bit old version due to some reasons, but I will migrate it : )

komamitsu · 2024-06-06T05:30:21Z

core/src/main/java/com/scalar/db/api/GetBuilder.java

+      implements Consistency<BuildableGetWithIndexWhere>, Projection<BuildableGetWithIndexWhere> {
+
+    protected BuildableGetWithIndex buildableGetWithIndex;
+    protected final SelectionBuilder.Where where;


Same as above

Fixed in 527d0b6.

komamitsu

LGTM! 👍

josh-wong

I've left some minor suggestions. Other than that, LGTM. Thank you!🙇🏻‍♂️

josh-wong · 2024-06-10T02:52:29Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+  GET_BUILD_ERROR_OPERATION_SUPPORTED_ONLY_WHEN_NO_CONDITIONS_ARE_SPECIFIED(
+      Category.USER_ERROR,
+      "0141",
+      "This operation is supported only when no conditions are specified at all. "


Suggested change

"This operation is supported only when no conditions are specified at all. "

"This operation is supported only when no conditions are specified. "

Thank you for the feedback. Fixed in e9840a9, including a similar message for the scan operation.

josh-wong · 2024-06-10T02:53:17Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

+      Category.USER_ERROR,
+      "0141",
+      "This operation is supported only when no conditions are specified at all. "
+          + "If you want to modify the condition, please use clearConditions() to remove all existing conditions first",


Suggested change

+ "If you want to modify the condition, please use clearConditions() to remove all existing conditions first",

+ "If you want to modify a condition, please use clearConditions() to remove all existing conditions first",

Ditto, fixed in e9840a9.

feeblefakie

Overall, looking good to me!
Left one minor comment. PTAL!

feeblefakie · 2024-06-10T03:55:38Z

core/src/main/java/com/scalar/db/api/Get.java

@@ -133,6 +133,11 @@ public Get withProjections(Collection<String> projections) {
    return (Get) super.withProjections(projections);
  }

+  @Override
+  Get withConjunctions(Collection<Conjunction> conjunctions) {


Should we add conjunctions to the toString() method?

Good catch, thank you! Fixed in e9840a9.

brfrn169

Left a minor comment, but other than that, LGTM! Thank you!

brfrn169 · 2024-06-11T13:50:35Z

core/import_schema.json

It looks like this file is unnecessary?

Sorry, my bad. Removed in d6d6475.

jnmt · 2024-06-14T06:35:22Z

@feeblefakie PTAL again when you get a chance. I will submit the following two related PRs once this is merged. Thank you!

exploiting partition scan/get filtering in ScalarDB SQL
fixing a snapshot management issue in ScalarDB

feeblefakie

LGTM! Thank you!

Add arbitrary filtering for get operations

7d6e1e7

jnmt commented Jun 4, 2024

View reviewed changes

jnmt self-assigned this Jun 4, 2024

jnmt added the enhancement New feature or request label Jun 4, 2024

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 June 4, 2024 08:08

Torch3333 approved these changes Jun 6, 2024

View reviewed changes

komamitsu reviewed Jun 6, 2024

View reviewed changes

jnmt added 3 commits June 7, 2024 11:25

Fix scopes

527d0b6

Merge branch 'master' into get-with-any-conditions

cdb9b96

Fix integration tests for the single-op transaction

3cc49fc

jnmt requested a review from komamitsu June 7, 2024 02:59

komamitsu approved these changes Jun 7, 2024

View reviewed changes

brfrn169 requested a review from josh-wong June 7, 2024 11:09

josh-wong approved these changes Jun 10, 2024

View reviewed changes

feeblefakie reviewed Jun 10, 2024

View reviewed changes

jnmt added 2 commits June 11, 2024 16:46

Merge branch 'master' into get-with-any-conditions

58bcfe2

Fix based on feedback

e9840a9

jnmt requested a review from feeblefakie June 11, 2024 08:03

brfrn169 approved these changes Jun 11, 2024

View reviewed changes

jnmt added 2 commits June 12, 2024 09:10

Remove unnecessary file

d6d6475

Merge branch 'master' into get-with-any-conditions

4fb7b24

feeblefakie approved these changes Jun 14, 2024

View reviewed changes

feeblefakie merged commit 197ac95 into master Jun 14, 2024
27 checks passed

feeblefakie deleted the get-with-any-conditions branch June 14, 2024 11:51

feeblefakie pushed a commit that referenced this pull request Jun 14, 2024

Add arbitrary filtering for get operations (#1834)

0be123d

feeblefakie mentioned this pull request Jun 14, 2024

Backport to branch(3) : Add arbitrary filtering for get operations #1906

Merged

jnmt mentioned this pull request Jun 21, 2024

Fix snapshot management issues #1976

Merged

6 tasks

jnmt mentioned this pull request Nov 25, 2024

Fix GetBuilder and ScanBuilder #2352

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add arbitrary filtering for get operations #1834

Add arbitrary filtering for get operations #1834

jnmt commented Jun 4, 2024 •

edited

Loading

jnmt Jun 4, 2024 •

edited

Loading

jnmt Jun 4, 2024 •

edited

Loading

jnmt Jun 4, 2024

jnmt Jun 4, 2024

Torch3333 left a comment

komamitsu Jun 6, 2024

jnmt Jun 7, 2024

komamitsu Jun 6, 2024

jnmt Jun 7, 2024

komamitsu left a comment

josh-wong left a comment

josh-wong Jun 10, 2024

jnmt Jun 11, 2024

josh-wong Jun 10, 2024

jnmt Jun 11, 2024

feeblefakie left a comment

feeblefakie Jun 10, 2024

jnmt Jun 11, 2024

brfrn169 left a comment •

edited

Loading

brfrn169 Jun 11, 2024

jnmt Jun 12, 2024

jnmt commented Jun 14, 2024

feeblefakie left a comment

		DistributedTransaction transaction =
		manager.begin(Isolation.SERIALIZABLE, SerializableStrategy.EXTRA_READ);


		public static class BuildableGetWithWhere extends BuildableGet {

		protected final SelectionBuilder.Where where;

	protected final SelectionBuilder.Where where;
	final SelectionBuilder.Where where;

	"This operation is supported only when no conditions are specified at all. "
	"This operation is supported only when no conditions are specified. "

	+ "If you want to modify the condition, please use clearConditions() to remove all existing conditions first",
	+ "If you want to modify a condition, please use clearConditions() to remove all existing conditions first",

Add arbitrary filtering for get operations #1834

Add arbitrary filtering for get operations #1834

Conversation

jnmt commented Jun 4, 2024 • edited Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

jnmt Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

jnmt Jun 4, 2024 • edited Loading

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

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

komamitsu left a comment

Choose a reason for hiding this comment

josh-wong 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

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

brfrn169 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnmt commented Jun 14, 2024

feeblefakie left a comment

Choose a reason for hiding this comment

jnmt commented Jun 4, 2024 •

edited

Loading

jnmt Jun 4, 2024 •

edited

Loading

jnmt Jun 4, 2024 •

edited

Loading

brfrn169 left a comment •

edited

Loading