Add like operator #1046

jnmt · 2023-08-30T02:47:18Z

This PR adds the like operator for the relational scan. In the LIKE conditional expression, you can specify an SQL-LIKE-like pattern and an escape character (optional). The following specification of wild cards and escape characters is basically the same as PostgreSQL and MySQL.

_ matches any single character
% matches any sequence of zero or more characters
\ works as the escape character by default
Escape character can be disabled by specifying an empty escape character

jnmt · 2023-08-31T00:49:15Z

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

+   *
+   * @return true if the LIKE pattern matches a given {@code String}
+   */
+  public boolean isMatchedWith(String value) {


We need our own matcher in the ScalarDB layer to avoid scan-after-write properly in Snapshot. The matcher uses regular expressions converted from "like" patterns.

jnmt · 2023-08-31T00:53:41Z

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

+    if (escape.isEmpty()) {
+      // MySQL accepts an empty escape character to disable the escape function, but MariaDB ignores


We need a tricky conversion for MySQL and MariaDB to handle both in the same manner since we don't want to distinguish them at this moment. But, in the future, we probably support the MariaDB driver and introduce RdbEngineMariadb or something like that.

This is just an idea, but how about giving up disabling escape on ScalarDB side like MariaDB does? In this way, users would need to use the default escape or specify a single escape character, but can't specify an empty string.

Thank you for the idea. That's one of the options I thought. However, all other databases other than MariaDB support disabling the escape, and it's probably convenient for users. So, can we go with it as is?

jnmt · 2023-08-31T00:55:07Z

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

+    if (escape.isEmpty()) {
+      // Even if users do not want to use escape character in ScalarDB (i.e., specifying "" for


We also need special care for SQL Server due to its dialect.

komamitsu · 2023-08-31T02:38:51Z

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

+  LikeExpression(TextColumn column, Operator operator, String escape) {
+    super(column, operator);
+    if (operator != Operator.LIKE && operator != Operator.NOT_LIKE) {
+      throw new IllegalArgumentException("Operator must be like or not-like");


[minor] How about adding the received unexpected operator in the error message to make debugging easier?

Fixed in 9654df0.

komamitsu · 2023-08-31T02:43:39Z

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

+   * @param escape an escape character.
+   * @return the equivalent Java regular expression of the given pattern
+   */
+  private String convertRegexPatternFrom(String likePattern, Character escape) {


Suggested change

private String convertRegexPatternFrom(String likePattern, Character escape) {

private String convertRegexPatternFrom(String likePattern, @Nullable Character escape) {

escape can be null, right?
Probably an additional import for this is needed.

Fixed in 9654df0, but removed when fixing others in 0c14b83.

komamitsu · 2023-08-31T04:27:36Z

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

+@Immutable
+public class LikeExpression extends ConditionalExpression {
+  private static final String DEFAULT_ESCAPE_CHAR = "\\";
+  @Nonnull private final String escape;


[trivial] IIUC, instance variables are regarded as non-null by default and we can remove this @Nonnull...?

Fixed in 9654df0.

komamitsu · 2023-08-31T04:37:48Z

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

+   * @param operator an operator used to compare the target column
+   * @param escape an escape character for the like operator
+   */
+  LikeExpression(TextColumn column, Operator operator, String escape) {


It seems escape is basically used as a single length string (or empty.) How about simply treating it as a nullable character? It would work as same as empty string if it's null.

@Nullable private final Character escape;

That's a good point. I also considered using Character as you suggested. But, in that case, users need to pass null to disable the escape character, right? (e.g., .isLikeText("%xyz", null)) This is because we want to use the default escape char \ for .isLikeText("%xyz"). I prefer passing an empty string "" rather than null since its semantics is the same as SQL. What do you think?

users need to pass null to disable the escape character, right? (e.g., .isLikeText("%xyz", null))
Yes.

I understand your point to use consistent semantics with SQL. I think I was confused a bit because com.scalar.db.storage.jdbc.RdbEngineStrategy#getEscape returns null while com.scalar.db.api.LikeExpression#getEscape returns an empty. How about making RdbEngineStrategy#getEscape return an empty as well as LikeExpression#getEscape?

After reading your comment #1046 (comment), I think my suggestion above was wrong... Please ignore it.

I prefer passing an empty string "" rather than null since its semantics is the same as SQL.
👍

Maybe #1046 (comment) is also related. It would be great if you take care of it.

Maybe #1046 (comment) is also related. It would be great if you take care of it.

Sure. I will revise it. Thanks for the feedback.

komamitsu · 2023-08-31T04:45:28Z

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

+   *
+   * @param column a target column used to compare
+   * @param operator an operator used to compare the target column
+   * @param escape an escape character for the like operator


Passing empty string looks to disable escapes. How about describing the behavior?

Fixed in 9654df0.

komamitsu · 2023-08-31T05:19:42Z

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

+    if (escape.isEmpty()) {
+      // MySQL accepts an empty escape character to disable the escape function, but MariaDB ignores


This is just an idea, but how about giving up disabling escape on ScalarDB side like MariaDB does? In this way, users would need to use the default escape or specify a single escape character, but can't specify an empty string.

komamitsu · 2023-08-31T05:24:26Z

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

+  @Override
+  public String getEscape(LikeExpression likeExpression) {
+    String escape = likeExpression.getEscape();
+    return escape.isEmpty() ? null : escape;


Is this also needed in RdbEnginePostgresql?

Moving this logic to com.scalar.db.storage.jdbc.RdbEngineStrategy#getEscape might be an option.

No, we always need the escape clause for PostgreSQL (if it's empty, we need to return the empty string as is). But for Oracle and SQLite, we need to remove the escape clause to disable the escape character.

brfrn169

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

brfrn169 · 2023-09-01T06:17:01Z

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

+   * @param escape an escape character.
+   * @return the equivalent Java regular expression of the given pattern
+   */
+  private String convertRegexPatternFrom(String likePattern, @Nullable Character escape) {


Can we move this logic into Snapshot since it's only used there?

The regular expression is only used in Snapshot, so technically, yes. But I think we should check if a specified pattern is an expected one because throwing an exception with "pattern must not ..." from Snapshot is weird. So, the choices are 1) keeping this as is and 2) only adding the check here. IMHO, 1) is not so bad since the check and conversion are tightly coupled with. What do you think?
p.s. isMatchedWith can be moved to Snapshot in any case.

I think we should choose the 2) option. We should separate the regular expression thing from LikeExpression because the responsibility of the regular expression thing is not for LikeExpression but Consensus Commit (e.g., JdbcTransaction doesn't use the regular expression). What do you think?

because the responsibility of the regular expression thing is not for LikeExpression but Consensus Commit (e.g., JdbcTransaction doesn't use the regular expression).

Right. Totally agreed. Thanks for pointing it out!

Fixed in 0c14b83. Note that I tried to simply check it with String.matched(regex), but it's hard to describe the expected one, and it will be error-prone for some corner cases when escaping the specified escape character (e.g., "++text" with the escape "+" is a possible pattern but "+++text" is not). So, I added a similar check to the original one, although it seems a bit exaggerated.

brfrn169 · 2023-09-01T06:17:14Z

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

+   *
+   * @return true if the LIKE pattern matches a given {@code String}
+   */
+  public boolean isMatchedWith(String value) {


Ditto. Can we move this logic into Snapshot since it's only used there?

Fixed in 0c14b83.

brfrn169 · 2023-09-01T06:32:23Z

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

@@ -133,6 +134,19 @@ public void visit(BlobColumn column) {
    }
  }

+  public void bindLikeClause(LikeExpression likeExpression) {


Hmm, we can't use the visitor pattern only for the LIKE operation. Honestly, I don't find it ideal, but I'm out of alternatives, and maybe it's acceptable...

Yeah, we have the same feelings...

brfrn169 · 2023-09-01T06:34:51Z

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

+        if ((condition.getOperator().equals(Operator.LIKE)
+            || condition.getOperator().equals(Operator.NOT_LIKE))) {
+          binder.bindLikeClause((LikeExpression) condition);
+        } else if (!condition.getColumn().hasNullValue()) {


This isn't directly related to this PR, but do we need to bind a NULL value when condition.getColumn().hasNullValue() returns true, similar to what PreparedStatementBinder does?

We don't need bindings for the columns with null values in conjunctions because the string IS (NOT) NULL is used for Operator.IS(_NOT)_NULL. But I'm not sure we should also use bind markers for them. Do you have any idea about that?

Yeah, that makes sense. We don't need to bind null values in conjunctions. Thanks.

komamitsu

LGTM! 👍

brfrn169

Left several minor suggestions, but LGTM! Thank you!

brfrn169 · 2023-09-05T03:36:46Z

...gration-test/java/com/scalar/db/storage/cosmos/CosmosConditionalMutationIntegrationTest.java

@@ -37,6 +37,9 @@ protected Map<String, String> getCreationOptions() {
  protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() {


This is a minor suggestion. I think we can modify this in the following way, ensuring we won't need any adjustments for similar changes in the future:

@Override protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() { return super.getOperatorAndDataTypeListForTest().stream() .filter( operatorAndDataType -> { // Cosmos DB only supports the 'equal' and 'not equal' and 'is null' and 'is not null' // conditions for BLOB type if (operatorAndDataType.getDataType() == DataType.BLOB) { return operatorAndDataType.getOperator() == Operator.EQ || operatorAndDataType.getOperator() == Operator.NE || operatorAndDataType.getOperator() == Operator.IS_NULL || operatorAndDataType.getOperator() == Operator.IS_NOT_NULL; } return true; }) .collect(Collectors.toList()); }

brfrn169 · 2023-09-05T03:37:22Z

...gration-test/java/com/scalar/db/storage/dynamo/DynamoConditionalMutationIntegrationTest.java

@@ -27,6 +27,9 @@ protected Map<String, String> getCreationOptions() {
  protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() {


Ditto.

@Override protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() { return super.getOperatorAndDataTypeListForTest().stream() .filter( operatorAndDataType -> { // DynamoDB only supports the 'equal' and 'not equal' and 'is null' and 'is not null' // conditions for BOOLEAN type if (operatorAndDataType.getDataType() == DataType.BOOLEAN) { return operatorAndDataType.getOperator() == Operator.EQ || operatorAndDataType.getOperator() == Operator.NE || operatorAndDataType.getOperator() == Operator.IS_NULL || operatorAndDataType.getOperator() == Operator.IS_NOT_NULL; } return true; }) .collect(Collectors.toList()); }

brfrn169 · 2023-09-05T03:46:10Z

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

+    if (likePattern == null) {
+      throw new IllegalArgumentException("LIKE pattern must not be null");
+    }


Since the validation for likePattern is already done here, I think using assertions is more appropriate:

Suggested change

if (likePattern == null) {

throw new IllegalArgumentException("LIKE pattern must not be null");

}

assert likePattern != null : "LIKE pattern must not be null";

brfrn169 · 2023-09-05T03:46:29Z

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

+        } else if (nextChar == escape) {
+          out.append(Pattern.quote(Character.toString(nextChar)));
+        } else {
+          throw new IllegalArgumentException("LIKE pattern must not include only escape character");


Ditto.

Suggested change

throw new IllegalArgumentException("LIKE pattern must not include only escape character");

throw new AssertionError("LIKE pattern must not include only escape character");

brfrn169 · 2023-09-05T03:46:36Z

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

+          throw new IllegalArgumentException("LIKE pattern must not include only escape character");
+        }
+      } else if (escape != null && c == escape) {
+        throw new IllegalArgumentException("LIKE pattern must not end with escape character");


Suggested change

throw new IllegalArgumentException("LIKE pattern must not end with escape character");

throw new AssertionError("LIKE pattern must not end with escape character");

jnmt · 2023-09-05T06:51:53Z

@brfrn169 Thank you for the additional feedback! I fixed all in 4aabb08.

feeblefakie

LGTM! Thank you!

Torch3333

Besides a minor suggestion, LGTM, thank you!

core/src/test/java/com/scalar/db/storage/jdbc/query/QueryBuilderTest.java

…erTest.java Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

Co-authored-by: Jun Nemoto <jun.nemoto@gmail.com> Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

jnmt added 2 commits August 30, 2023 11:46

Add like operator

efcb67f

Fix to handle MariaDB

88edb9e

jnmt commented Aug 31, 2023

View reviewed changes

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 August 31, 2023 00:56

jnmt self-assigned this Aug 31, 2023

jnmt added the enhancement New feature or request label Aug 31, 2023

komamitsu reviewed Aug 31, 2023

View reviewed changes

Fix based on feedback

9654df0

brfrn169 reviewed Sep 1, 2023

View reviewed changes

Fix based on feedback

0c14b83

jnmt requested review from komamitsu and brfrn169 September 4, 2023 13:22

komamitsu approved these changes Sep 5, 2023

View reviewed changes

brfrn169 approved these changes Sep 5, 2023

View reviewed changes

Fix based on feedback

4aabb08

feeblefakie approved these changes Sep 6, 2023

View reviewed changes

Torch3333 approved these changes Sep 7, 2023

View reviewed changes

core/src/test/java/com/scalar/db/storage/jdbc/query/QueryBuilderTest.java Outdated Show resolved Hide resolved

jnmt and others added 3 commits September 8, 2023 08:55

Update core/src/test/java/com/scalar/db/storage/jdbc/query/QueryBuild…

000a15e

…erTest.java Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

Merge branch 'master' into add-like-operator

07c4e97

Merge branch 'master' into add-like-operator

299413e

jnmt merged commit e7ae27e into master Sep 8, 2023
13 checks passed

jnmt deleted the add-like-operator branch September 8, 2023 01:20

brfrn169 pushed a commit that referenced this pull request Oct 26, 2023

Add like operator (#1046)

e7e8f4b

Co-authored-by: Jun Nemoto <jun.nemoto@gmail.com> Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

brfrn169 pushed a commit that referenced this pull request Oct 26, 2023

Add like operator (#1046)

3e9ccb7

Co-authored-by: Jun Nemoto <jun.nemoto@gmail.com> Co-authored-by: Vincent Guilpain <vincent.guilpain@scalar-labs.com>

		if (escape.isEmpty()) {
		// MySQL accepts an empty escape character to disable the escape function, but MariaDB ignores

		if (escape.isEmpty()) {
		// Even if users do not want to use escape character in ScalarDB (i.e., specifying "" for

	private String convertRegexPatternFrom(String likePattern, Character escape) {
	private String convertRegexPatternFrom(String likePattern, @Nullable Character escape) {

		@@ -37,6 +37,9 @@ protected Map<String, String> getCreationOptions() {
		protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() {

		@@ -27,6 +27,9 @@ protected Map<String, String> getCreationOptions() {
		protected List<OperatorAndDataType> getOperatorAndDataTypeListForTest() {

	throw new IllegalArgumentException("LIKE pattern must not include only escape character");
	throw new AssertionError("LIKE pattern must not include only escape character");

Add like operator #1046

Add like operator #1046

Conversation

jnmt commented Aug 30, 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

komamitsu Aug 31, 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

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

komamitsu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnmt commented Sep 5, 2023

feeblefakie left a comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

jnmt commented Aug 30, 2023 •

edited

komamitsu Aug 31, 2023 •

edited