QL: case sensitive support in EQL by astefan · Pull Request #56404 · elastic/elasticsearch

astefan · 2020-05-08T07:46:21Z

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.

* 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

…o 55340_enhancement3

elasticmachine · 2020-05-08T07:46:24Z

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

matriv

Looks good, left some comments mostly minor.

docs/reference/sql/functions/string.asciidoc

.../src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/StartsWith.java

x-pack/plugin/eql/src/main/resources/org/elasticsearch/xpack/eql/plugin/eql_whitelist.txt

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/analysis/VerifierTests.java

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionPipeTests.java

x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt

x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec

matriv · 2020-05-10T11:20:52Z

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionProcessor.java

+    private final boolean isCaseSensitive;

-    public StartsWithFunctionProcessor(Processor source, Processor pattern) {
+    public StartsWithFunctionProcessor(Processor source, Processor pattern, boolean isCaseSensitive) {


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

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

matriv · 2020-05-10T11:21:11Z

...ava/org/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionPipe.java

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

 public class StartsWithFunctionPipe extends Pipe {


Maybe remove Function from the name.

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderOkTests.java

costin

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

costin · 2020-05-11T16:24:28Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/PlanExecutor.java

 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;


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

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.

...org/elasticsearch/xpack/sql/expression/function/scalar/NoArgumentsConfigurationFunction.java

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/session/EqlSession.java

.../src/main/java/org/elasticsearch/xpack/eql/expression/function/scalar/string/StartsWith.java

…o 55340_enhancement3

matriv

LGTM. left 2 minor formatting comments.

matriv · 2020-05-12T11:05:13Z

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionPipeTests.java

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


Suggested change

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

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

matriv · 2020-05-12T11:06:01Z

...rg/elasticsearch/xpack/ql/expression/function/scalar/string/StartsWithFunctionPipeTests.java

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


Suggested change

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

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

…o 55340_enhancement3

* 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)

* 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)

astefan added 4 commits May 7, 2020 23:34

* StartsWith is case sensitive aware

e6b0d19

* 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

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

be3be7a

…o 55340_enhancement3

Add case sensitive aware queryfolder tests

4fbbec0

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

166c232

…o 55340_enhancement3

astefan added >enhancement :Analytics/SQL SQL querying :Analytics/EQL EQL querying labels May 8, 2020

astefan requested review from aleksmaus, costin, matriv and rw-access May 8, 2020 07:46

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

matriv reviewed May 10, 2020

View reviewed changes

costin reviewed May 11, 2020

View reviewed changes

x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderOkTests.java Show resolved Hide resolved

costin approved these changes May 11, 2020

View reviewed changes

astefan added 2 commits May 12, 2020 11:38

Address reviews

b39a6e4

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

fc59f14

…o 55340_enhancement3

astefan requested a review from matriv May 12, 2020 08:39

matriv approved these changes May 12, 2020

View reviewed changes

astefan added 2 commits May 12, 2020 14:18

Address review elastic#2

5c81621

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

df628dc

…o 55340_enhancement3

astefan merged commit ee5a09e into elastic:master May 12, 2020

astefan deleted the 55340_enhancement3 branch May 12, 2020 12:06

astefan added the backport pending label May 12, 2020

astefan mentioned this pull request May 12, 2020

QL: case sensitive support in EQL (#56404) #56597

Merged

astefan removed the backport pending label May 12, 2020

astefan mentioned this pull request May 13, 2020

EQL: Fix cidrMatch function fails to match when used in scripts #56246

Merged

jrodewig mentioned this pull request May 13, 2020

[DOCS] EQL: Document case_sensitive param #56697

Merged

rw-access mentioned this pull request May 14, 2020

EQL: Handle case insensitivity for Comparisons #56771

Closed

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

		public class StartsWithFunctionPipe extends Pipe {

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

Conversation

astefan commented May 8, 2020

Uh oh!

elasticmachine commented May 8, 2020

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv May 10, 2020

Choose a reason for hiding this comment

Uh oh!

astefan May 12, 2020

Choose a reason for hiding this comment

Uh oh!

matriv May 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

costin May 11, 2020

Choose a reason for hiding this comment

Uh oh!

astefan May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

matriv May 12, 2020

Choose a reason for hiding this comment

Uh oh!

matriv May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants