Refactor scan with filtering #1715

jnmt · 2024-05-08T08:17:26Z

Description

This PR refactors ScanAll with a where clause so that we can support Get, GetWithIndex Scan, and ScanWithIndex with a where clause in a consistent way in the upcoming updates. After careful discussion, we decided to introduce a few backward-incompatible changes since cross-partition scan with filtering is a relatively new feature and we expected their users to be none or less.

Related issues and/or PRs

Support dynamic arbitrary filtering on NoSQL databases #1682 (upcoming after merging this)

Changes made

Move Conjunction from Scan to Selection
Move ConditionSetBuilder and {And,Or}ConditionSet from ScanBuilder to api
Refactor ScanBuilder
- Rename BuildableScanAllFromExistingWithX to BuildableScanFromExistingWithX so that it can be commonly used for Scan, ScanWithIndex and ScanAll
- Consolidate member variables in BuildableScanFromExistingWithWhere using a composite buildable
- Introduce OngoingWhere for the common internal condition management

Checklist

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

The following PRs will be upcoming in this order.

Dynamic filtering for ScalAll in Support dynamic arbitrary filtering on NoSQL databases #1682
Dynamic filtering for Scan and ScanWithIndex
Dynamic filtering for Get and GetWithIndex

Release notes

Refactored scan with filtering.

jnmt · 2024-05-08T08:58:20Z

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

-      AndConditionSet other = (AndConditionSet) o;
-      return conditions.equals(other.conditions);
-    }
+  private static class OngoingWhere {


This is introduced for simply and commonly managing the internal condition state for both BuildableScanAllWithWhere and BuildableScanFromExistingWithXX. Since it can also be used for GetBuilder, this class may be moved to somewhere commonplace.

Also, I avoided using just Where to distinguish the Where interface, but the name might be a bit weird since it will also be used after closing the where clause. InternalWhere, WhereClause, or WhereBuilder? What do you think?

WhereBuilder sounds to me that it has build() method. So, it might be worth adding build() that returns Collection<Conjunction> or something.

WhereClause itself sounds good to me, but I noticed the term Clause isn't used in this file...

I think WhereCondition might be an option as the class contains only conditions.

Thanks for the suggestion. I changed my mind a bit. Since the interface Where is only used twice in this file, using it with the modifier (i.e., OperationBuilder.Where) is not so bad looking. I'm going with this if no further concerns.

jnmt · 2024-05-08T09:02:49Z

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

@@ -621,9 +591,9 @@ public static class BuildableScanOrScanAllFromExisting extends BuildableScan
          OperationBuilder.Table<BuildableScanOrScanAllFromExisting>,


Since BuildableScanOrScanAllFromExisting is also used for changing an existing ScanWithIndex, it might be better BuildableScanAllFromExisting. But BuildableScanOrScanAllFromExisting is older than the cross-partition scan with filtering, so I think breaking it would be a bit aggressive. Please let me know if you have any opinions.

jnmt · 2024-05-08T12:30:16Z

Sorry, I found some null checks were accidentally deleted when refactoring. Fixed in 797b95e.

komamitsu · 2024-05-09T01:57:08Z

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

+     */
+    public BuildableAndConditionSet and(ConditionalExpression condition) {
+      conditions.add(condition);
+      return new BuildableAndConditionSet(conditions);


How about copying conditions or using immutable conditions to prevent the shared mutable collection from unexpectedly being changed?

Right, that's well-mannered. Is the following expected?

@Immutable public static class BuildableAndConditionSet { private final Set<ConditionalExpression> conditions; BuildableAndConditionSet(Set<ConditionalExpression> conditions) { this.conditions = ImmutableSet.copyOf(conditions); } public BuildableAndConditionSet and(ConditionalExpression condition) { return new BuildableAndConditionSet( new ImmutableSet.Builder<ConditionalExpression>() .addAll(conditions) .add(condition) .build()); }

How about copying conditions or using immutable conditions to prevent the shared mutable collection from unexpectedly being changed?

I think my comment was a bit confusing. You could simply return just this if you choose the mutate builder pattern. You can also take immutable approach. Mutable builder pattern is still common, so either would be fine.

For immutable way, probably you can declare conditions as ImmutableSet while it's not an interface. This might be controversial, but I think it's acceptable since Java doesn't have immutable collection interface.

scalardb/core/src/main/java/com/scalar/db/api/Selection.java

Lines 146 to 152 in 16b7a09

@Immutable

public static class Conjunction {

private final ImmutableSet<ConditionalExpression> conditions;

private Conjunction(ImmutableSet<ConditionalExpression> conditions) {

this.conditions = conditions;

}

would be a good example.

Thanks! Unfortunately, we cannot just return this here since we need to prevent users from calling or once they start with and. So, recreating a set without affecting the current set seems the best solution here.

Ah, you're right. I thought it as a usual builder pattern...

Also, I think I was thinking of defensive copy and immutability when I writing the first comment, but I forgot it when writing the second comment. Sorry for the confusion.

As we talked on another chat, immutable approach is basically better as long as there is no memory memory-pressure concerns.

komamitsu · 2024-05-09T02:16:01Z

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

-      this.orderings.addAll(buildable.orderings);
-      this.projections.addAll(buildable.projections);
-      this.consistency = buildable.consistency;
+    BuildableScanOrScanAllFromExisting buildableScanFromExisting;


Suggested change

BuildableScanOrScanAllFromExisting buildableScanFromExisting;

private BuildableScanOrScanAllFromExisting buildableScanFromExisting;

This can be private?

komamitsu · 2024-05-09T02:19:03Z

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

-      this.projections.addAll(buildable.projections);
-      this.consistency = buildable.consistency;
+    BuildableScanOrScanAllFromExisting buildableScanFromExisting;
+    protected final OngoingWhere where;


IntelliJ shows Class 'OngoingWhere' is exposed outside its defined visibility scope. So, this should be package-private?

Suggested change

protected final OngoingWhere where;

final OngoingWhere where;

komamitsu · 2024-05-09T02:32:22Z

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

-      AndConditionSet other = (AndConditionSet) o;
-      return conditions.equals(other.conditions);
-    }
+  private static class OngoingWhere {


WhereBuilder sounds to me that it has build() method. So, it might be worth adding build() that returns Collection<Conjunction> or something.

WhereClause itself sounds good to me, but I noticed the term Clause isn't used in this file...

I think WhereCondition might be an option as the class contains only conditions.

jnmt · 2024-05-09T10:55:56Z

@komamitsu Thank you for a lot of valuable feedback. I fixed them in a23097e. Also, I've added a little further refactoring in
1a0d6e5. PTAL when you get a chance.

komamitsu · 2024-05-10T02:14:52Z

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

-    protected final List<Scan.Ordering> orderings = new ArrayList<>();
-    protected int limit;
-    @Nullable protected com.scalar.db.api.Consistency consistency;
+    protected BuildableScanAll buildableScanAll;


[trivial] BuildableScanAll is mutable, so you can also make this variable final and simply use it like this.

@Override public BuildableScanAllWithWhere projection(String projection) { buildableScanAll.projection(projection); return this; }

The current code can be used as-is if BuildableScanAll changes into immutable in the future, so I don't have a strong opinion on it , though.

Thanks again. Definitely. Fixed them in 8320ae8 as you mentioned due to no plan for changing them to immutable so far.

komamitsu

LGTM! Thank you!

brfrn169

LGTM! Thank you!

Torch3333

LGTM, thank you!

feeblefakie

LGTM! Thank you!
(Very sorry for the late review 🙇 )

jnmt and others added 3 commits May 8, 2024 10:44

Refactor scan filtering

16b7a09

Refactor maintaining ongoing where

19df8c2

Merge branch 'master' into refactor-scan-filtering

2921302

jnmt changed the title ~~Refactor scan filtering~~ Refactor scan with filtering May 8, 2024

jnmt commented May 8, 2024

View reviewed changes

jnmt self-assigned this May 8, 2024

jnmt added the bugfix label May 8, 2024

jnmt marked this pull request as ready for review May 8, 2024 09:15

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 May 8, 2024 09:16

Fix null check

797b95e

Further refactoring using composition

1a0d6e5

komamitsu reviewed May 9, 2024

View reviewed changes

Fix based on feedback

a23097e

jnmt requested a review from komamitsu May 9, 2024 10:56

komamitsu reviewed May 10, 2024

View reviewed changes

Fix based on feedback

8320ae8

jnmt requested a review from komamitsu May 10, 2024 05:11

komamitsu approved these changes May 10, 2024

View reviewed changes

brfrn169 added improvement and removed bugfix labels May 13, 2024

brfrn169 approved these changes May 13, 2024

View reviewed changes

Torch3333 approved these changes May 14, 2024

View reviewed changes

feeblefakie approved these changes May 14, 2024

View reviewed changes

Merge branch 'master' into refactor-scan-filtering

69b22f0

brfrn169 merged commit ce3440f into master May 14, 2024
23 checks passed

brfrn169 deleted the refactor-scan-filtering branch May 14, 2024 13:08

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

Refactor scan with filtering (#1715)

4aafd79

This was referenced May 14, 2024

Backport to branch(3.12) : Refactor scan with filtering #1729

Merged

Backport to branch(3.11) failed: Refactor scan with filtering #1730

Closed

Backport to branch(3.10) failed: Refactor scan with filtering #1731

Closed

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

Refactor scan with filtering (#1715)

c21d244

feeblefakie mentioned this pull request May 14, 2024

Backport to branch(3) : Refactor scan with filtering #1732

Merged

jnmt added a commit that referenced this pull request May 15, 2024

Refactor scan with filtering (#1715)

e29fb79

jnmt added a commit that referenced this pull request May 15, 2024

Refactor scan with filtering (#1715)

8908268

This was referenced May 15, 2024

Backport to branch(3.10) : Refactor scan with filtering #1733

Merged

Backport to branch(3.11) : Refactor scan with filtering #1734

Merged

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

Refactor scan with filtering #1715

Refactor scan with filtering #1715

jnmt commented May 8, 2024 •

edited

Loading

jnmt May 8, 2024

jnmt May 8, 2024

komamitsu May 9, 2024

jnmt May 9, 2024

jnmt May 8, 2024

jnmt commented May 8, 2024

komamitsu May 9, 2024

jnmt May 9, 2024

komamitsu May 9, 2024

jnmt May 9, 2024

komamitsu May 9, 2024

komamitsu May 9, 2024

komamitsu May 9, 2024

komamitsu May 9, 2024

jnmt commented May 9, 2024

komamitsu May 10, 2024

jnmt May 10, 2024

komamitsu left a comment

brfrn169 left a comment

Torch3333 left a comment

feeblefakie left a comment

		@@ -621,9 +591,9 @@ public static class BuildableScanOrScanAllFromExisting extends BuildableScan
		OperationBuilder.Table<BuildableScanOrScanAllFromExisting>,

	@Immutable
	public static class Conjunction {
	private final ImmutableSet<ConditionalExpression> conditions;

	private Conjunction(ImmutableSet<ConditionalExpression> conditions) {
	this.conditions = conditions;
	}

	BuildableScanOrScanAllFromExisting buildableScanFromExisting;
	private BuildableScanOrScanAllFromExisting buildableScanFromExisting;

	protected final OngoingWhere where;
	final OngoingWhere where;

Refactor scan with filtering #1715

Refactor scan with filtering #1715

Conversation

jnmt commented May 8, 2024 • edited Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

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

jnmt commented May 8, 2024

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

jnmt commented May 9, 2024

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

brfrn169 left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

jnmt commented May 8, 2024 •

edited

Loading