Skip to content
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

QL: case sensitive support in EQL #56404

Merged
merged 8 commits into from
May 12, 2020
Merged

Conversation

astefan
Copy link
Contributor

@astefan astefan commented May 8, 2020

This PR adds several enhancements:

  • adds a generic startsWith function to QL
  • modifies the existent EQL startsWith function to be case sensitive aware
  • improves the existent EQL startsWith function to use a prefix query when the function is used in a case sensitive context. Same improvement is used in SQL's newly added STARTS_WITH function.
  • adds case sensitivity to EQL configuration through a case_sensitive parameter in the eql request, as established in EQL: case sensitivity in ES EQL string functions #54411. The case_sensitive parameter can be specified when running queries (default is case insensitive)

What this PR is not covering:

  • case sensitivity in other string related EQL functions
  • case sensitivity aware integration testing in EQL

Fixes #55340.
Relates to #54411.

astefan added 4 commits May 7, 2020 23:34
* Added case sensitivity to EQL configuration
* case_sensitive parameter can be specified when running queries (default
is case insensitive)
* Added STARTS_WITH function to SQL as well
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label May 8, 2020
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.

Looks good, left some comments mostly minor.

docs/reference/sql/functions/string.asciidoc Show resolved Hide resolved

public StartsWithFunctionProcessor(Processor source, Processor pattern) {
public StartsWithFunctionProcessor(Processor source, Processor pattern, boolean isCaseSensitive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove the Function as we don't normally use it in processor names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are functions where this pattern is used - String functions.

@@ -17,13 +17,15 @@

public class StartsWithFunctionPipe extends Pipe {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove Function from the name.

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.

Left a few comments but looks good to me otherwise.
Thanks for the various cleanups.

@@ -22,7 +22,7 @@
import org.elasticsearch.xpack.sql.planner.Planner;
import org.elasticsearch.xpack.sql.planner.PlanningException;
import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;
import org.elasticsearch.xpack.sql.session.Configuration;
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of using Configuration instead of SqlConfiguration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a rename. The SQL's Configuration => SqlConfiguration.
The same for Eql. EQL's Configuration was renamed to EqlConfiguration.

I found it a bit confusing to have Configuration in three separate projects.

@astefan astefan requested a review from matriv May 12, 2020 08:39
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. left 2 minor formatting comments.

pipe(((Expression) randomValueOtherThan(f.field(), () -> randomStringLiteral()))),
pipe(((Expression) randomValueOtherThan(f.pattern(), () -> randomStringLiteral()))),
f.isCaseSensitive()));
for(int i = 1; i < 4; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for(int i = 1; i < 4; i++) {
for (int i = 1; i < 4; i++) {

pipe(((Expression) randomValueOtherThan(f.pattern(), () -> randomStringLiteral()))),
f.isCaseSensitive()));
for(int i = 1; i < 4; i++) {
for(BitSet comb : new Combinations(3, i)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for(BitSet comb : new Combinations(3, i)) {
for (BitSet comb : new Combinations(3, i)) {

@astefan astefan merged commit ee5a09e into elastic:master May 12, 2020
@astefan astefan deleted the 55340_enhancement3 branch May 12, 2020 12:06
astefan added a commit to astefan/elasticsearch that referenced this pull request May 12, 2020
* adds a generic startsWith function to QL
* modifies the existent EQL startsWith function to be case sensitive
aware
* improves the existent EQL startsWith function to use a prefix query
when the function is used in a case sensitive context. Same improvement
is used in SQL's newly added STARTS_WITH function.
* adds case sensitivity to EQL configuration through a case_sensitive
parameter in the eql request, as established in elastic#54411.
The case_sensitive parameter can be specified when running queries
(default is case insensitive)

(cherry picked from commit ee5a09e)
astefan added a commit that referenced this pull request May 12, 2020
* QL: case sensitive support in EQL (#56404)
* adds a generic startsWith function to QL
* modifies the existent EQL startsWith function to be case sensitive
aware
* improves the existent EQL startsWith function to use a prefix query
when the function is used in a case sensitive context. Same improvement
is used in SQL's newly added STARTS_WITH function.
* adds case sensitivity to EQL configuration through a case_sensitive
parameter in the eql request, as established in #54411.
The case_sensitive parameter can be specified when running queries
(default is case insensitive)

(cherry picked from commit ee5a09e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying :Analytics/SQL SQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QL: Optimize startWith to prefix queries where possible
4 participants