Skip to content

SQL: Fix issue with LIKE/RLIKE as painless script #53495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 12, 2020

Add missing asScript() implementation for LIKE/RLIKE expressions.

When LIKE/RLIKE are used for example in GROUP BY or are wrapped with
scalar functions in a WHERE clause, the translation must produce a
painless script which will be executed to implement the correct
behaviour and previously this was completely missing, and as a
consquence wrong results were silently (no error) returned.

Fixes: #53486

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

@matriv
Copy link
Contributor Author

matriv commented Mar 12, 2020

I decided on a Pattern iface because I didn't like to use LikePattern#toString() => asJavaRegex, I felt it could be confusing, but on the other hand it kind of feels a bit "too much" to add that iface.

Add missing asScript() implementation for LIKE/RLIKE expressions.

When LIKE/RLIKE are used for example in GROUP BY or are wrapped with
scalar functions in a WHERE clause, the translation must produce a
painless script which will be executed to implement the correct
behaviour and previously this was completely missing, and as a
consquence wrong results were silently (no error) returned.

Fixes: elastic#53486
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

*/
package org.elasticsearch.xpack.ql.expression.predicate.regex;

interface Pattern {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe RegexPattern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, the LikePattern is not 100% regex pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about StringMatchPattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not StringPattern ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure that too!

}

@Override
public ScriptTemplate asScript() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matriv matriv merged commit eaa8ead into elastic:master Mar 16, 2020
@matriv matriv deleted the fix-53486 branch March 16, 2020 10:55
matriv added a commit that referenced this pull request Mar 16, 2020
Add missing asScript() implementation for LIKE/RLIKE expressions.

When LIKE/RLIKE are used for example in GROUP BY or are wrapped with
scalar functions in a WHERE clause, the translation must produce a
painless script which will be executed to implement the correct
behaviour and previously this was completely missing, and as a
consquence wrong results were silently (no error) returned.

Fixes: #53486
(cherry picked from commit eaa8ead)
matriv added a commit that referenced this pull request Mar 16, 2020
Add missing asScript() implementation for LIKE/RLIKE expressions.

When LIKE/RLIKE are used for example in GROUP BY or are wrapped with
scalar functions in a WHERE clause, the translation must produce a
painless script which will be executed to implement the correct
behaviour and previously this was completely missing, and as a
consquence wrong results were silently (no error) returned.

Fixes: #53486
(cherry picked from commit eaa8ead)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: LIKE/RLIKE don't produce painless scripts
6 participants