Skip to content

QL: Optimize Like/Rlike all #62682

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 24, 2020
Merged

QL: Optimize Like/Rlike all #62682

merged 5 commits into from
Sep 24, 2020

Conversation

costin
Copy link
Member

@costin costin commented Sep 21, 2020

Replace common Like and RLike queries that match all characters with
IsNotNull (exists) queries

Fix #62585

@costin
QL: Optimize Like/Rlike all
1febf98 
Replace common Like and RLike queries that match all characters with
IsNotNull (exists) queries

Fix elastic#62585
@costin costin requested review from astefan and matriv September 21, 2020 06:35
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Sep 21, 2020
matriv
matriv reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Left a comment about the Like/LikePattern.

@costin
Copy link
Member Author

costin commented Sep 21, 2020

I've added more tests to check the query translation and it turns out IsNull/IsNotNull always defaulted to scripts. So I've fixed the translation rules and added moved the wildcard pattern rule in the optimizer so that according to the comments in #62651:

  1. it only gets applied once
  2. it ignores folding and works only on literal strings

It's worth revisiting some of the translations since this might not be singular case.

matriv
matriv reviewed Sep 21, 2020
View reviewed changes
...ugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/regex/LikePattern.java Outdated
@Override
public boolean matchesAll() {
for (int i = 0 ; i < pattern.length(); i++) {
if (pattern.charAt(i) != '%') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that now the "problem" is reversed since the ReplaceWildcards takes place before this rule, but the test seems to work: https://github.com/elastic/elasticsearch/pull/62682/files#diff-6dd3fd1d0809ee5978cdccbc6bb7d7e1R17 ? In any case I think we should have a comment mentioning the order of the rules are applied and the choice of % or * in this rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

LikePattern has several styles. In its constructor it accepts the % and _ characters instead of * and ?. The conversion is done internally and accessible.
The check above works on the raw pattern - I'll follow-up with a comment.

@matriv
Copy link
Contributor

matriv commented Sep 22, 2020

Relates to #62760

@costin
Copy link
Member Author

costin commented Sep 23, 2020

@elasticmachine update branch

astefan
astefan approved these changes Sep 23, 2020
View reviewed changes
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

@costin costin merged commit 4c23fad into elastic:master Sep 24, 2020
@costin costin deleted the fix/62585 branch September 24, 2020 10:32
@costin costin added the v8.0.0 label Sep 24, 2020
costin added a commit that referenced this pull request Sep 24, 2020
@costin
QL: Optimize Like/Rlike all (#62682)
71b92f8 
Replace common Like and RLike queries that match all characters with
IsNotNull (exists) queries

Fix #62585

(cherry picked from commit 4c23fad)
matriv
matriv reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM, nice approach with the new checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matriv matriv matriv left review comments

@astefan astefan astefan approved these changes

Assignees
No one assigned
Labels
:Analytics/EQL EQL querying :Analytics/SQL SQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EQL: Optimization for string with single wildcard
5 participants
@costin @elasticmachine @matriv @astefan @jakelandis