Add an interface to add conditions in ScanAll #889

jnmt · 2023-05-24T04:08:10Z

This PR adds an interface to add arbitrary conditions in ScanAll, i.e., to support a relational scan for JDBC databases. We decided to add them to the existing ScanAll instead of adding a new RelationalScan to keep the backward compatibility and simplicity of the current class inheritance. Actual relational scan processing will be introduced by another PR.

By this PR, users can add a kind of "where()" clause after calling "all()" in a Scan builder. We currently support only a disjunctive normal form (DNF) or a conjunctive normal form (CNF), like the following patterns.

.where( conditionalExpression )
.where( conditionalExpression | andConditionSet ) [ .or( conditionalExpression | andConditionSet ) ] ...
.where( conditionalExpression | orConditionSet ) [ .and( conditionalExpression | orConditionSet ) ] ...

Users can use ConditionSetBuilder to build AndConditionSet and OrCondtionSet, which is a set of conjunctive (a && b && ...) or disjunctive (a || b || ...) conditions. Note that users need to call clearConditions() before updating a where clause in an existing ScallAll object.

See also inline comments for details.

jnmt · 2023-05-24T04:30:10Z

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

@@ -34,6 +37,7 @@ public class Scan extends Selection {
  private Optional<Key> endClusteringKey;
  private boolean endInclusive;
  private int limit;
+  private final Set<Conjunction> conjunctions;


Internally, we use a disjunctive set of conjunctions (i.e., as a DNF) to manage whole conditions.

jnmt · 2023-05-24T04:37:04Z

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

+      scan.withConjunctions(
+          Sets.cartesianProduct(disjunctions).stream()
+              .map(Scan.Conjunction::of)
+              .collect(Collectors.toSet()));


When building a where clause with ".and().and()...", the builder holds a (conjunctive) set of disjunctions, and its conjunction should be empty. In this case, we convert CNF to DNF according to the distributive law.

jnmt · 2023-05-24T04:38:31Z

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

+              .map(Scan.Conjunction::of)
+              .collect(Collectors.toSet()));
+    } else {
+      scan.withConjunctions(conjunctions);


In contrast, when building a where clause with ".or().or()...", the builder naturally holds conditions as a DNF. So, it just adds them.

jnmt · 2023-05-24T04:45:46Z

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

   */
  @Deprecated
  @Override
  public ScanAll withOrdering(Ordering ordering) {
-    throw new UnsupportedOperationException();
+    return (ScanAll) super.withOrdering(ordering);


Unlike a current ScanAll, a relational scan accept orderings. So, now we need to implement this, although we would like to use this internally for the builder and users do not have to use this.

It's not guaranteed that super.withOrdering() returns ScanAll in terms of the type signature and it seems an unsafe downcast to me. I'm wondering if any type check is needed (or applying Generics type parameter, but it takes effort...) ?

I thought it was safe because the caller object of super.withOrdering() here is always ScanAll (and we are already doing the same in other similar methods).
@brfrn169 What do you think?

Technically, it is not safe. If we change the withOrdering() method of the superclass Scan so that it does not return this, a ClassCastException may be thrown. However, at present, Scan.withOrdering() does return this, and since we have control over the Scan class, I am comfortable with the downcast.

@komamitsu What do you think?

Yeah, it seems there is no reasonable solution to it. I think we can go with the implementation as is.

I wish Java had a special generics type THIS or something which represents the concrete class type itself so that we could write the method like public THIS withOrdering()...

@brfrn169 @komamitsu Thank you! I left this as is.

komamitsu · 2023-05-25T02:18:56Z

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

   */
  @Deprecated
  @Override
  public ScanAll withOrdering(Ordering ordering) {
-    throw new UnsupportedOperationException();
+    return (ScanAll) super.withOrdering(ordering);


It's not guaranteed that super.withOrdering() returns ScanAll in terms of the type signature and it seems an unsafe downcast to me. I'm wondering if any type check is needed (or applying Generics type parameter, but it takes effort...) ?

komamitsu · 2023-05-25T02:38:55Z

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

+  }
+
+  @Immutable
+  public static class AndConditionSet {


Sorry, Conjunction and AndConditionSet sound similar to me. Could you add a comment to clarify what they are ?

Good point. Essentially, both are the same. But I intentionally used the different classes for the internal representation (Conjunction) and user API (AndConditionSet) for ease of understanding since the conjunction would be unfamiliar for many users. From the developer's perspective, I just prefer the term "conjunction" since it's short and technically correct to use many places internal modules, including gRPC proto. Do you think "internal use" or something like that is enough here? Or should we use AndConditionSet also for the internal representation?

So maybe we should move AndConditionSet and OrConditionSet and their builder classes into ScanBuilder?

I see. Thanks for the explanation. Either option, adding a comment that describes the relationship between Conjunction and AndConditionSet and the usages or using AndConditionSet for internal use, sounds fine with me.

@komamitsu @brfrn169 Thanks for the comments. I will move both sets to the builder and describe the relationship and usage.

Fixed in d3bb194.

brfrn169

I reviewed it briefly and left some comments. As we should examine it thoroughly, let me review it again later.

brfrn169 · 2023-05-26T02:22:08Z

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

+  Scan withConjunctions(Collection<Conjunction> conjunctions) {
+    this.conjunctions.addAll(conjunctions);
+    return this;
+  }


Because we are going to make Scan an immutable class, please don't add a mutation method or make it deprecated?

I misunderstand something, but this method is package private. Is "deprecated" still necessary? I imagined current public withFooBar() mutation methods would be changed to package private ones like this function in v5.0.0.

@brfrn169 Just a confirmation. Will those mutation methods themselves be deleted in v5.0.0? In addition, for example, Scan will have ScanBuilder, and the builder can directly access the private member of Scan without using withFooBar(). That's why we should make it deprecated even if a package private method, is my guess correct?

Understood. Since the method in question is package-private and not public, there's no need to remove it or mark it as deprecated. Let's keep it as it is. If further changes or considerations are needed in the future, we can revisit this decision.

brfrn169 · 2023-05-26T02:30:06Z

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

+     * @return set of {@code ConditionalExpression} which this conjunction is composed of
+     */
+    public Set<ConditionalExpression> getConditions() {
+      return ImmutableSet.copyOf(conditions);


Since the private constructor always gets ImmutableSet from the static factory methods, we don't need to copy the conditions here.

Suggested change

return ImmutableSet.copyOf(conditions);

return conditions;

Thank you for pointing it out. Fixed in d3bb194.

brfrn169 · 2023-05-26T02:32:56Z

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

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


If the private constructor always gets ImmutableSet from the static factory methods, we might be able to make it ImmutableSet to make it more explicit.

Suggested change

private final Set<ConditionalExpression> conditions;

private final ImmutableSet<ConditionalExpression> conditions;

@komamitsu What do you think?

Agreed. It's a good improvement in terms of type consistency.

Thank you for pointing it out. Fixed in d3bb194.

brfrn169 · 2023-05-26T02:38:22Z

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

+
+    private final Set<ConditionalExpression> conditions;
+
+    AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {


For consistency, we can make it private:

Suggested change

AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {

private AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {

Good catch, thank you! Fixed in d3bb194.

brfrn169 · 2023-05-26T03:09:23Z

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

   */
  @Deprecated
  @Override
  public ScanAll withOrdering(Ordering ordering) {
-    throw new UnsupportedOperationException();
+    return (ScanAll) super.withOrdering(ordering);


Technically, it is not safe. If we change the withOrdering() method of the superclass Scan so that it does not return this, a ClassCastException may be thrown. However, at present, Scan.withOrdering() does return this, and since we have control over the Scan class, I am comfortable with the downcast.

@komamitsu What do you think?

brfrn169 · 2023-05-26T03:10:44Z

core/src/test/java/com/scalar/db/api/ScanTest.java

+  //  private Set<Conjunction> prepareConjunctions() {
+  //    return ImmutableSet.of(
+  //        RelationalScan.Conjunction.conjunction(
+  //            ConditionBuilder.column(ANY_NAME_1).isEqualToText(ANY_TEXT_1)),
+  //        RelationalScan.Conjunction.conjunction(
+  //            ConditionBuilder.column(ANY_NAME_1).isEqualToText(ANY_TEXT_2)));
+  //  }
+  //
+  //  private Set<Conjunction> prepareConjunctionsWithDifferentOrder() {
+  //    return ImmutableSet.of(
+  //        RelationalScan.Conjunction.conjunction(
+  //            ConditionBuilder.column(ANY_NAME_1).isEqualToText(ANY_TEXT_2)),
+  //        RelationalScan.Conjunction.conjunction(
+  //            ConditionBuilder.column(ANY_NAME_1).isEqualToText(ANY_TEXT_1)));
+  //  }


Should we remove this commented out code?

Oh, sorry. Fixed in d3bb194.

brfrn169 · 2023-05-26T03:58:40Z

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

+    private BuildableScanAllWithWhere(String namespaceName, String tableName) {
+      this.namespaceName = namespaceName;
+      this.tableName = tableName;
+      this.condition = null;
+    }
+
+    private BuildableScanAllWithWhere(
+        String namespaceName, String tableName, ConditionalExpression condition) {
+      this.namespaceName = namespaceName;
+      this.tableName = tableName;
+      this.condition = condition;
+    }


Suggested change

private BuildableScanAllWithWhere(String namespaceName, String tableName) {

this.namespaceName = namespaceName;

this.tableName = tableName;

this.condition = null;

}

private BuildableScanAllWithWhere(

String namespaceName, String tableName, ConditionalExpression condition) {

this.namespaceName = namespaceName;

this.tableName = tableName;

this.condition = condition;

}

private BuildableScanAllWithWhere(String namespaceName, String tableName) {

this(namespaceName, tableName, null);

}

private BuildableScanAllWithWhere(

String namespaceName, String tableName, @Nullable ConditionalExpression condition) {

this.namespaceName = namespaceName;

this.tableName = tableName;

this.condition = condition;

}

Thank you for the advice. Fixed in d3bb194.

brfrn169 · 2023-05-26T04:27:39Z

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

+        throw new UnsupportedOperationException(
+            "This operation is supported only when no conditions are specified at all.");


Instead of UnsupportedOperationException, I think IllegalStateException is proper in this case. And I think we can suggest using clearConditions() to clear the conditions in the message:

Suggested change

throw new UnsupportedOperationException(

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

throw new IllegalStateException(

"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.");

Exactly. Fixed in d3bb194.

brfrn169 · 2023-05-26T04:35:07Z

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

+  }
+
+  @Immutable
+  public static class AndConditionSet {


So maybe we should move AndConditionSet and OrConditionSet and their builder classes into ScanBuilder?

jnmt · 2023-05-29T05:27:15Z

@brfrn169 @komamitsu Thank you for the valuable feedback. I fixed all other than this. PTAL again when you get a chance.

komamitsu · 2023-05-29T07:14:45Z

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

+    protected final Set<Scan.Conjunction> conjunctions = new HashSet<>();
+    protected final List<Set<ConditionalExpression>> disjunctions = new ArrayList<>();


Sorry for this delayed question. Why the types of conjunctions and disjunctions are different (disjunctions needs an order? disjunctions should be an internal class as well as conjunctions?)

Thanks for the question. Actually, disjunctions don't need the order information, but Sets.cartesianProduct(), which is used for converting a CNF, requires a list.

I see. If the reason of having different types for conjunctions and disjunctions is only for Sets.cartesianProduct(), we can write like this?

Sets.cartesianProduct( disjunctions.stream().map(Disjunction::getConditions).collect(Collectors.toList())) .stream() .map(Conjunction::of) .collect(Collectors.toSet()));

It would be great if the types of conjunctions and disjunctions are the same-level abstraction because I guess the readers of this code expect that.

Yeah, that's better for readers. Thank you. I think we can do just like the following. BTW, I'm not going to introduce Scan.Disjunction since the CNF and disjunctions are only used in the ScanBuilder internally at least at this moment. Even so, is it weird and should we have it?

scan.withConjunctions( Sets.cartesianProduct(new ArrayList<>(disjunctions)).stream() .map(Scan.Conjunction::of) .collect(Collectors.toSet()));

BTW, I'm not going to introduce Scan.Disjunction since the CNF and disjunctions are only used in the ScanBuilder internally at least at this moment. Even so, is it weird and should we have it?

@komamitsu Or, how about using Set<Set<ConditionalExpression>> for conjunctions when building and converting them at the last, similar to the disjunctions? What do you think?

how about using Set<Set> for conjunctions

It means like this?

protected final Set<Set<ConditionalExpression>> conjunctions = new HashSet<>(); protected final Set<Set<ConditionalExpression>> disjunctions = new HashSet<>();

If so, completely agreed.

Yeah, that's what I meant. Fixed in a64862d. PTAL when you get a chance.

…ions

…isjunction

jnmt · 2023-05-30T08:29:03Z

Dear all,
Sorry, I missed some cases for specifying orderings without conditions, updating a namespace and table after the where clause, and clearing the namespace after the where clause. Fixed in 2c305c9. PTAL, including those.

brfrn169

LGTM! Thank you!

Torch3333

LGTM, thank you!

komamitsu · 2023-06-01T02:00:47Z

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

+      scan.withConjunctions(ImmutableSet.of(Scan.Conjunction.of(condition)));
+    } else if (conjunctions.isEmpty()) {
+      scan.withConjunctions(
+          Sets.cartesianProduct(new ArrayList<>(disjunctions)).stream()


(Sorry for the delayed comment. I thought I put the following comment here, but it's gone...)

This seems to generate O x A conditions (O sets of A conditions), where O is the number of ORs and A is the number of ANDs. I'm not sure how these sets of conditions would be used in next PRs, but I guess ScalarDB fetches all entries and then filter the fetched records using these conditions set.

(This would be a silly question..., but) it's not possible to pushdown original conditions basically as-is to underlying RDBMS?

Thank you for the question. It will be used in something like the following way. We don't filter results using the conditions (at least for JDBC storage) in the ScalarDB layer.

private String relationalConditionSqlString() { (snip) List<String> conjunctionList = new ArrayList<>(); conjunctions.forEach( conjunction -> { List<String> conditions = new ArrayList<>(); conjunction .getConditions() .forEach( condition -> conditions.add(condition.getColumn().getName() + convert(condition.getOperator()))); conjunctionList.add(String.join(" AND ", conditions)); }); return " WHERE " + String.join(" OR ", conjunctionList); }

(This would be a silly question..., but) it's not possible to pushdown original conditions basically as-is to underlying RDBMS?

Do you mean pushing down both DNF and CNF as is? It's technically possible, and it sometimes would perform better, but I think it trades off a development effort and maintainability for two internal representations.

Thanks for the answer. It sounds good!

What I meant was...
converting the following Scan

Scan.newBuilder() .namespace("my_ns") .table("my_tbl") .all() .where(ConditionBuilder.column("c1").isEqualToInt(10)) .and(ConditionBuilder.column("c2").isEqualToBigInt(20)) .and( ConditionSetBuilder.condition( ConditionBuilder.column("c3").isEqualToInt(30)) .or(ConditionBuilder.column("c4").isEqualToInt(40)) .build()) : .build();

into the following SQL

SELECT ... FROM my_ns.my_tbl WHERE c1 = 10 AND c2 = 20 AND (c3 = 30 OR c4 = 40)

although I'm not sure it works well 😅

I see. That's pushing down a CNF as is. Yeah, as I said, technically, it works if we make Scan always have either conjunctions or disjunctions. But my concern is that we probably need to do the same for the gRPC proto and take care of which one should be used in many places. Anyway, this is kind of an internal part; we can revisit this in the future. Any additional discussions are welcome. Thanks for the comment.

komamitsu

LGTM! 👍

feeblefakie

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

feeblefakie

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

Add an interface to add conditions in ScanAll

d4e9cb5

jnmt added the enhancement New feature or request label May 24, 2023

jnmt self-assigned this May 24, 2023

jnmt commented May 24, 2023

View reviewed changes

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 May 24, 2023 04:50

jnmt mentioned this pull request May 24, 2023

Add operation interfaces for upcoming relational abstraction support #842

Closed

komamitsu reviewed May 25, 2023

View reviewed changes

komamitsu self-requested a review May 25, 2023 03:02

brfrn169 reviewed May 26, 2023

View reviewed changes

Fix based on feedback

d3bb194

jnmt requested a review from brfrn169 May 29, 2023 05:27

komamitsu reviewed May 29, 2023

View reviewed changes

jnmt added 2 commits May 30, 2023 11:53

Add interfaces for update/clear namespace and ordering without condit…

2c305c9

…ions

Fix to use sets of conditional expressions for both conjunction and d…

a64862d

…isjunction

jnmt requested a review from komamitsu May 30, 2023 08:25

Merge branch 'master' into add-condition-interface-for-scanall

c9ea54b

brfrn169 approved these changes May 31, 2023

View reviewed changes

Torch3333 approved these changes May 31, 2023

View reviewed changes

komamitsu reviewed Jun 1, 2023

View reviewed changes

komamitsu approved these changes Jun 1, 2023

View reviewed changes

feeblefakie approved these changes Jun 7, 2023

View reviewed changes

feeblefakie merged commit 1bd01a5 into master Jun 7, 2023
12 checks passed

feeblefakie deleted the add-condition-interface-for-scanall branch June 7, 2023 02:26

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

Add an interface to add conditions in ScanAll (#889)

379d5a5

brfrn169 mentioned this pull request Nov 28, 2023

Rename Relational Scan to Cross-partition Scan #1329

Merged

6 tasks

	private final Set<ConditionalExpression> conditions;
	private final ImmutableSet<ConditionalExpression> conditions;


		private final Set<ConditionalExpression> conditions;

		AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {

	AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {
	private AndOrConditionSetBuilder(Set<ConditionalExpression> conditions) {

		throw new UnsupportedOperationException(
		"This operation is supported only when no conditions are specified at all.");

-        throw new UnsupportedOperationException(
-            "This operation is supported only when no conditions are specified at all.");
+        throw new IllegalStateException(
+            "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.");

		protected final Set<Scan.Conjunction> conjunctions = new HashSet<>();
		protected final List<Set<ConditionalExpression>> disjunctions = new ArrayList<>();

Add an interface to add conditions in ScanAll #889

Add an interface to add conditions in ScanAll #889

Conversation

jnmt commented May 24, 2023 • edited

jnmt May 24, 2023 • edited

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

brfrn169 left a comment

Choose a reason for hiding this comment

brfrn169 May 26, 2023 • edited

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

brfrn169 May 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnmt commented May 29, 2023

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 30, 2023

brfrn169 left a 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 • edited

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

jnmt commented May 24, 2023 •

edited

jnmt May 24, 2023 •

edited

brfrn169 May 26, 2023 •

edited

brfrn169 May 26, 2023 •

edited

komamitsu left a comment •

edited