SQL: Escaped wildcard (*) not accepted in LIKE #63428

palesz · 2020-10-07T16:53:45Z

For a query like SELECT name FROM test WHERE name LIKE '%c*' ES SQL generates an error. * is not a special character in a LIKE construct and it's expected to not needing to be escaped, so the previous query should work as is.
In the LIKE pattern any * character was treated as invalid character and the usage of % or _ was suggested instead. But * is a valid, acceptable non-wildcard on the right side of the LIKE operator.

Fix: #55108

elasticmachine · 2020-10-07T16:53:48Z

Pinging @elastic/es-ql (:Query Languages/SQL)

palesz · 2020-10-07T17:10:35Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

-        int pos = pattern.indexOf('*');
-        if (pos >= 0) {
-            throw new ParsingException(source(ctx.value),
-                    "Invalid char [*] found in pattern [{}] at position {}; use [%] or [_] instead",
-                    pattern, pos);
-        }


@costin Did I miss anything, is there a reason to keep this check here?

To prevent folks from using * instead of %. Whether that's still a concern I don't know.

We don't have anything in the docs regarding the * and we list only the % and _
* should be interpreted as a normal char for SQL:

postgres=# create table mytable (str varchar(10)); CREATE TABLE postgres=# insert into mytable values('*test*'), ('as*test*fd'); INSERT 0 2 postgres=# select * from mytable where str like ('%*test*%'); str ------------ *test* as*test*fd (2 rows) postgres=# select * from mytable where str like ('*test*'); str -------- *test* (1 row)

How about adding a NOTE in the docs to explicitly specify that *, ? and the usual regex chars are interpreted as normal chars?

AFAIK it is only the MS Access SQL flavour that treats * as a wildcard in a LIKE pattern, so SQL folks should be fairly used to * not being a wildcard. ES customers with less SQL knowledge might not. I think adding a NOTE to the docs does not hurt.

I am also thinking about removing the restriction on not being able to use * as an ESCAPE character from here. Do you see any good reason to keep that restriction?
As a user * would not be my first choice as an ESCAPE character, but the parser can handle it since * is just a normal character.

On PostgreSQL, MySQL, SQLite any character can be used as ESCAPE (even wildchars) and any character can be escaped, not just wildcards. PostgreSQL treats the '%%' ESCAPE '%' as a non-wildcard % character (just as another escaped character), while MySQL and SQLite treat it as a wildcard (special escaping case). I assume it was intentional to behave differently and only allow escaping of the wildchars.

As I see it, since only % and _ are wildcards for LIKE I would only allow escaping of those, but if we go on with this change it shouldn't be part of the bugfix that would be backported, but for next minor release since it's a minor "breaking" change.

Agree @matriv that allowing escaping any characters should be a separate PR and likely should be in the next minor release only, although technically it is less of a breaking change unless someone depends on the exceptions around escaping non-wildchars. Anyways, this is off topic for this PR.

To clarify: in this current PR I am only removing the special treatment of * and the restriction on using *as an ESCAPE character (wildchars are still not allowed as ESCAPE characters). Does this sound good to you?

costin

Please do use Markdown as in the initial report (even more so when copy-pasting) - it improves readability a lot.

costin · 2020-10-07T17:07:11Z

...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/LikeEscapingParsingTests.java

@@ -29,7 +29,7 @@ private String error(String pattern) {
    }

    private LikePattern like(String pattern) {
-        Expression exp = null;
+        Expression exp;


Please revert.

Will do. I was wondering what is your convention.

Generally speaking, I don't have a strong preference for explicitly initialising as null but some times it helps for readability. In this case we have a clear if/else statement that assigns a value, so imho is not so important.

matriv

Looks good, Maybe add one more test with a more complicated string, something like:
*%*_string*_**%. Left one more comment about the error message.

matriv · 2020-10-07T17:10:35Z

...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/LikeEscapingParsingTests.java

        assertThat(error("'|*string' ESCAPE '|'"),
-                is("line 1:11: Invalid char [*] found in pattern [|*string] at position 1; use [%] or [_] instead"));
+            is("line 1:11: Pattern [|*string] is invalid as escape char [|] at position 0 can only escape wildcard chars; found [*]"));


Should we maybe improve the error message by listing the wildcard chars for LIKE: % and _ ?

Looks good, Maybe add one more test with a more complicated string, something like:
*%*_string*_**%. Left one more comment about the error message.

Will add a new test case, but I will add it to the LikeConversionTests, since I think that has less to do with ESCAPEing.

For a query like `SELECT name FROM test WHERE name LIKE ''%c*'` ES SQL generates an error. `*` is not a special character in a `LIKE` construct and it's expected to not needing to be escaped, so the previous query should work as is. In the LIKE pattern any `*` character was treated as invalid character and the usage of `%` or `_` was suggested instead. But `*` is a valid, acceptable non-wildcard on the right side of the `LIKE` operator. Fix: elastic#55108

palesz · 2020-10-07T21:02:51Z

Sorry for the force-push, I should not have done that.

matriv · 2020-10-08T09:33:46Z

docs/reference/sql/functions/like-rlike.asciidoc

@@ -33,8 +33,8 @@ with the `LIKE` operator:
 * The percent sign (%)
 * The underscore (_)

-The percent sign represents zero, one or multiple characters. The underscore represents a single number or character. These symbols can be
-used in combinations.
+No other characters have special meaning or act as wildcard. The percent sign represents zero, one or multiple characters.


Personally I'd add it as a [NOTE] below the existing sentence, so that it stands out.

nit: It might be worth providing first the explanation of the above chars, then add a clarification; i.e. reposition the first sentence at the end.

matriv · 2020-10-08T09:37:08Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

-        int pos = pattern.indexOf('*');
-        if (pos >= 0) {
-            throw new ParsingException(source(ctx.value),
-                    "Invalid char [*] found in pattern [{}] at position {}; use [%] or [_] instead",
-                    pattern, pos);
-        }


As I see it, since only % and _ are wildcards for LIKE I would only allow escaping of those, but if we go on with this change it shouldn't be part of the bugfix that would be backported, but for next minor release since it's a minor "breaking" change.

bpintea · 2020-10-08T10:17:11Z

docs/reference/sql/functions/like-rlike.asciidoc

@@ -33,8 +33,8 @@ with the `LIKE` operator:
 * The percent sign (%)
 * The underscore (_)

-The percent sign represents zero, one or multiple characters. The underscore represents a single number or character. These symbols can be
-used in combinations.
+No other characters have special meaning or act as wildcard. The percent sign represents zero, one or multiple characters.


nit: It might be worth providing first the explanation of the above chars, then add a clarification; i.e. reposition the first sentence at the end.

Added a NOTE explaining that only `%` and `_` treated as wildcards and other characters (even `*` or `?`) are just treated as normal characters in the `LIKE` pattern.

matriv

LGTM. Thx!

astefan

Too late with the review. Sorry about that. Fwiw LGTM, with a minor cosmetic comment. Thanks.

astefan · 2020-10-13T13:38:04Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

@@ -288,7 +282,7 @@ public LikePattern visitPattern(PatternContext ctx) {
            } else if (escapeString.length() == 1) {
                escape = escapeString.charAt(0);
                // these chars already have a meaning
-                if (escape == '*' || escape == '%' || escape == '_') {
+                if (escape == '%' || escape == '_') {
                    throw new ParsingException(source(escapeCtx.escape), "Char [{}] cannot be used for escaping", escape);


On the same idea (which I like) of actually mentioning the wildcard chars in the error message itself (for the lazy user who doesn't read the docs and asking "why % char cannot be used for escaping?"), what about adapting this error message to include the reason? Something like Char [%] cannot be used for escaping as it's one of the wildcard chars [%_].

Thanks, good suggestion. I'll add this cosmetic fix.

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates elastic#63428

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428

For a query like `SELECT name FROM test WHERE name LIKE ''%c*'` ES SQL generates an error. `*` is not a special character in a `LIKE` construct and it's expected to not needing to be escaped, so the previous query should work as is. In the LIKE pattern any `*` character was treated as invalid character and the usage of `%` or `_` was suggested instead. But `*` is a valid, acceptable non-wildcard on the right side of the `LIKE` operator. Fix: #55108 (cherry picked from commit 190d9fe)

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428 (cherry picked from commit 74cbcf8)

For a query like `SELECT name FROM test WHERE name LIKE ''%c*'` ES SQL generates an error. `*` is not a special character in a `LIKE` construct and it's expected to not needing to be escaped, so the previous query should work as is. In the LIKE pattern any `*` character was treated as invalid character and the usage of `%` or `_` was suggested instead. But `*` is a valid, acceptable non-wildcard on the right side of the `LIKE` operator. Fix: #55108 (cherry picked from commit 190d9fe)

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428 (cherry picked from commit 74cbcf8)

For a query like `SELECT name FROM test WHERE name LIKE ''%c*'` ES SQL generates an error. `*` is not a special character in a `LIKE` construct and it's expected to not needing to be escaped, so the previous query should work as is. In the LIKE pattern any `*` character was treated as invalid character and the usage of `%` or `_` was suggested instead. But `*` is a valid, acceptable non-wildcard on the right side of the `LIKE` operator. Fix: elastic#55108

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates elastic#63428

For a query like `SELECT name FROM test WHERE name LIKE ''%c*'` ES SQL generates an error. `*` is not a special character in a `LIKE` construct and it's expected to not needing to be escaped, so the previous query should work as is. In the LIKE pattern any `*` character was treated as invalid character and the usage of `%` or `_` was suggested instead. But `*` is a valid, acceptable non-wildcard on the right side of the `LIKE` operator. Fix: #55108 Backports #63616 #63428

palesz added >bug :Analytics/SQL SQL querying v8.0.0 v7.10.0 v7.11.0 labels Oct 7, 2020

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 7, 2020

palesz requested review from astefan and matriv October 7, 2020 16:54

costin added v7.10.1 and removed v7.10.0 labels Oct 7, 2020

palesz commented Oct 7, 2020

View reviewed changes

costin reviewed Oct 7, 2020

View reviewed changes

matriv reviewed Oct 7, 2020

View reviewed changes

palesz force-pushed the fix/55108 branch from 50fc9d5 to c368506 Compare October 7, 2020 20:53

Merge branch 'master' of github.com:elastic/elasticsearch into fix/55108

5c97c9d

matriv reviewed Oct 8, 2020

View reviewed changes

bpintea approved these changes Oct 8, 2020

View reviewed changes

Andras Palinkas added 3 commits October 8, 2020 11:58

Merge remote-tracking branch 'origin/master' into fix/55108

f089b47

Documentation changes for SQL LIKE as per PR

5e6bc1c

Added a NOTE explaining that only `%` and `_` treated as wildcards and other characters (even `*` or `?`) are just treated as normal characters in the `LIKE` pattern.

Fixed the checkstyle errors

ff5ee18

matriv approved these changes Oct 8, 2020

View reviewed changes

palesz merged commit 190d9fe into elastic:master Oct 13, 2020

astefan approved these changes Oct 13, 2020

View reviewed changes

palesz pushed a commit to palesz/elasticsearch that referenced this pull request Oct 13, 2020

SQL: Improve parser error message for ESCAPE

2b8272b

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates elastic#63428

palesz mentioned this pull request Oct 13, 2020

SQL: Improve parser error message for ESCAPE #63616

Merged

palesz pushed a commit that referenced this pull request Oct 13, 2020

SQL: Improve parser error message for ESCAPE (#63616)

74cbcf8

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428

palesz added the backport pending label Oct 13, 2020

palesz pushed a commit that referenced this pull request Oct 13, 2020

SQL: Improve parser error message for ESCAPE (#63616)

dff4f98

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428 (cherry picked from commit 74cbcf8)

palesz pushed a commit that referenced this pull request Oct 13, 2020

SQL: Improve parser error message for ESCAPE (#63616)

f9c8ae5

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates #63428 (cherry picked from commit 74cbcf8)

palesz removed the backport pending label Oct 13, 2020

andreidan added v7.10.0 and removed v7.10.1 labels Oct 15, 2020

palesz pushed a commit to palesz/elasticsearch that referenced this pull request Oct 19, 2020

SQL: Improve parser error message for ESCAPE (elastic#63616)

be0717b

Mentions the list of wildchars in case a wildchar is used as an `ESCAPE` character. Relates elastic#63428

palesz mentioned this pull request Oct 19, 2020

SQL: Escaped wildcard (*) not accepted in LIKE #63906

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

SQL: Escaped wildcard (*) not accepted in LIKE #63428

SQL: Escaped wildcard (*) not accepted in LIKE #63428

Uh oh!

Conversation

palesz commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palesz Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palesz commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

palesz commented Oct 7, 2020 •

edited

Loading

palesz Oct 7, 2020 •

edited

Loading

matriv Oct 8, 2020 •

edited

Loading

matriv Oct 8, 2020 •

edited

Loading