SQL: Implement IN(value1, value2, ...) expression. #34581

matriv · 2018-10-18T10:42:26Z

Implement the functionality to translate the
`field IN (value1, value2,...) expressions to proper Lucene queries
or painless script depending on the use case.

The IN expression can be used in SELECT, WHERE and HAVING clauses.

Closes: #32955

Implement the functionality to translate the `field IN (value1, value2,...) expressions to proper Lucene queries or painless script depending on the use case. The `IN` expression can be used in SELECT, WHERE and HAVING clauses. Closes: elastic#32955

elasticmachine · 2018-10-18T10:42:28Z

Pinging @elastic/es-search-aggs

matriv · 2018-10-18T10:44:57Z

Currently there is no check for null handling. It will be done in a next step once #34573 is merged.
Tracked by #34582.

There are no tests for negation when there is a translation to painless as this: #34558 must be fixed first

astefan · 2018-10-18T15:04:02Z

I tested this a bit and a combination of a function and IN values of a different type (select * from test_emp where lcase(first_name) in (1,2)) would be incorrect, but the error message presented to the user is rather verbose and exposing the full stack trace of the error.

astefan · 2018-10-18T15:10:15Z

@matriv this one fails and I think it shouldn't:

sql> select * from test_emp where year(birth_date) in (1990);
Server error [Server sent bad type [search_phase_execution_exception]. Original type was [all shards failed]. [Failed to execute phase [query], all shards failed; shardFailures {[1xf7J15UQ6-6Eagq0pzBBg][test_emp][0]: RemoteTransportException[[node-0][127.0.0.1:9300][indices:data/read/search[phase/query]]]; nested: QueryShardException[failed to create query: {
  "script" : {
    "script" : {
      "source" : "InternalSqlScriptUtils.dateTimeChrono(doc[params.v0].value.millis, params.v1, params.v2)==1990",
      "lang" : "painless",
      "params" : {
        "v0" : "birth_date",
        "v1" : "UTC",
        "v2" : "YEAR"
      }
    },
    "boost" : 1.0
  }
}]; nested: ScriptException[compile error]; nested: ClassCastException[Cannot cast from [int] to [java.lang.Object].]; }
        at org.elasticsearch.action.search.AbstractSearchAsyncAction.onPhaseFailure(AbstractSearchAsyncAction.java:293)
        at org.elasticsearch.action.search.AbstractSearchAsyncAction.executeNextPhase(AbstractSearchAsyncAction.java:133)
        at org.elasticsearch.action.search.AbstractSearchAsyncAction.onPhaseDone(AbstractSearchAsyncAction.java:254)

costin · 2018-10-18T15:28:00Z

The above comparison is fixed in #34573 (the underlying null-safe equality does proper widening when comparing Numbers).

matriv · 2018-10-18T16:23:09Z

@astefan

I tested this a bit and a combination of a function and IN values of a different type (select * from test_emp where lcase(first_name) in (1,2)) would be incorrect, but the error message presented to the user is rather verbose and exposing the full stack trace of the error.

Thanks for catching that. Added validation and tests for nice error message.

astefan · 2018-10-18T17:06:24Z

sql> select * from test_emp where 1<2 in (false);
Bad request [Found 1 problem(s)
line 1:8: Unexecutable item]

Strange that the one above fails, but the next one doesn't: select * from test_emp where 1<2 in (true).

astefan

Left some comments, otherwise LGTM.

matriv · 2018-10-18T17:41:47Z

@astefan Thanks for digging in and testing!

select * from test_emp where 1<2 in (false);

This is a general problem, not related to in. Opened issue #34613.

costin

Looks good!
I've left a number of comments but most of them are stylistic.
I wonder if there's an optimization rule that we can use to removes from the list the items that are known to not match in order to minimize the list and thus the number of pipes, etc.. that follows.
That would only work though if the value (the left as you say) is constant and thus all not matching constants from the list could be removed:

`SELECT 1 IN (2,3, foo) FROM TABLE;

costin · 2018-10-18T19:32:26Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java

+        if (this == other) {
+            return true;
+        } else if (this.isString() && other.isString()) {


There's no need to specify this : if (isString() && other.isString())

costin · 2018-10-18T19:36:41Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -188,6 +193,16 @@ private static Failure fail(Node<?> source, String message, Object... args) {

                Set<Failure> localFailures = new LinkedHashSet<>();

+                if (p instanceof Filter) {


To increase readability please move the two ifs to a separate method (checkInExpression?) similar to checkGroupBy.

costin · 2018-10-18T19:38:02Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+            In in = (In) e;
+            DataType dt = null;
+            for (Expression rightValue : in.list()) {


rightValue is a bit confusing since it also means the correct value. How about inValue or just value?

costin · 2018-10-18T19:43:39Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+            DataType dt = null;
+            for (Expression rightValue : in.list()) {
+                if (dt == null) {


Maybe I'm misunderstanding the rule but I think this can be simplified by initializing dt with a first value and avoid the double ifs (which care very similar).
If the rule check the the values between themselves, this can be done by picking the first item and then comparing it with the rest (through an index (a bit verbose but fast) or a sublist).
If the rule checks the value against the list (which includes the former but can't be as precise in the message if the former occurs) dt is initialized to the In.value() and then iterates through the list.

costin · 2018-10-18T19:47:30Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

+
+    private static void validateInExpression(Expression e, Set<Failure> localFailures) {
+        if (e instanceof In) {


You can avoid the instanceof check using plan.forEachExpression(method, In.class) so for Filter you can do
filterPlan.condition().forEachExpression(validateIn, In.class)

validateIn(In in) { ...}
the Set<Failure> can be passed to the closure directly - see checkGroupBy & co for examples.

Nice! Thx for the suggestion.

costin · 2018-10-18T20:01:52Z

...c/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InPipe.java

+        for (Pipe p : right) {
+            newRight.add(p.resolveAttributes(resolver));
+        }
+        return replaceChildren(Stream.concat(Stream.of(newLeft), newRight.stream()).collect(Collectors.toList()));


Again, if there's only one list no need to separate, concatenate things.

costin · 2018-10-18T20:02:39Z

...c/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InPipe.java

+
+    @Override
+    public boolean resolved() {
+        return left().resolved() && right().stream().allMatch(Pipe::resolved);


Resolvables.resolved(list).

hm cannot use that. Pipe is not Resolvable.

Changing Pipe to implement Resolvable

costin · 2018-10-18T20:04:02Z

...c/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/InPipe.java

+
+public class InPipe extends Pipe {
+
+    private Pipe left;


I think left and right are a bit confusing - why not use value and list?

costin · 2018-10-18T20:06:03Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

+            // if the code gets here it's a bug
+            //
+            else {
+                throw new UnsupportedOperationException("No idea how to translate " + in.value());


Use SqlIllegalArgumentException instead.

Copy pasted from here: https://github.com/elastic/elasticsearch/pull/34581/files/ab7c1502d1b6341b2b3cc3be7eda72b00e606ada#diff-5d7529d13a2ef47d436ea2aa577e0c52R543 :-( Fixing both!

costin · 2018-10-18T20:11:08Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

-    private Analyzer analyzer;
-
-    public QueryTranslatorTests() {
+public class QueryTranslatorTests extends AbstractBuilderTestCase {


Is AbstractBuilderTestCase used anywhere (maybe I'm missing it)?

I used it for the shardContext here: https://github.com/elastic/elasticsearch/pull/34581/files/ab7c1502d1b6341b2b3cc3be7eda72b00e606ada#diff-aef2b0ce456b8fdd5cc09d6cfd55f0c2R173.
I tried to manually mock it but ended up with ugly code. The AbstractBuilderTestCase is parent class for many tests in .search.aggregations and .index.query packages.

costin · 2018-10-18T20:16:04Z

By the way, this should go in 6.5 as well.

The setting that reduces the disk space requirement for the forecasting integration tests was accidentally removed in elastic#31757 when files were moved around. This change simply adds back the setting that existed before that.

Applies our standard column wrapping to the `discovery-ec2` and `repository-s3` plugins.

Changes wording in the FIPS 140-2 related documentation. Co-authored-by: derickson <dave@elastic.co>

Adds support for query-time formatting of the date histo keys when executing a rollup search. Closes elastic#34391

costin

LGTM.
I've added some minor comments regarding styling.
Also, it's worth adding two unit tests : one to check the optimizer does folding of the in expressions (see optimizer tests - something like 1 in (2-1, 2, 3), it should return true and another to see whether In removes duplicates 1 in (1,2,3,1,2,3,1,2,3) which is handled by passing the list through an insertion-order set. From there on it can be treated as a list, knowing there are no duplicates.

costin · 2018-10-19T18:09:54Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryTranslator.java

@@ -144,7 +150,7 @@
    }

    static QueryTranslation toQuery(Expression e, boolean onAggs) {
-        QueryTranslation translation = null;
+        QueryTranslation translation;


Looks like a formatting issue - I'd keep the explicit initialization (to keep introspection tools at bay that yes, null is expected).

costin · 2018-10-19T18:11:37Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java

+    public TermsQuery(Location location, String term, List<Expression> values) {
+        super(location);
+        this.term = term;
+        this.values = values.stream().map(Expression::fold).collect(Collectors.toList());


the collector could be a set (as oppose to a list) to remove duplicates.

costin · 2018-10-19T18:15:02Z

...n/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/Comparisons.java

 /**
 * Comparison utilities.
 */
-abstract class Comparisons {
+public final class Comparisons {


Minor nitpick, I tend to explicitly use valueOf/xxxValue to clarify the use of boxing.

costin · 2018-10-19T18:16:49Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/In.java

    }

    @Override
    public boolean foldable() {
-        return foldable;
+        return children().stream().allMatch(Expression::foldable);


Can be moved to Expressions#foldable similar to nullable and resolvable.

matriv · 2018-10-20T00:05:12Z

@costin Addressed comments. Please take another look.

If the whole IN expression is not foldable then the values in the list IN(v1, v2, ...) are not folded because the ConstantFolding is a DOWN rule. This is not a problem as when we build the TermsQuery or the painless script we call fold().

Making the ConstantFolding an UP rule solves this more elegantly, but it generates a serious of other issues. Maybe we can introduce a special ConstantFolding rule just for the IN or you think the current solution is ok?

costin · 2018-10-21T15:01:41Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/TermsQuery.java

+    public TermsQuery(Location location, String term, List<Expression> values) {
+        super(location);
+        this.term = term;
+        this.values = values.stream().map(Expression::fold).collect(Collectors.toCollection(LinkedHashSet::new));


As an alternative, use Foldables.*: new LinkedHashSet(Foldables.valuesOf(values, datatType()))

costin

LGTM, thanks.

The optimization rule is not critical since the end result is the same though I'm curious why it doesn't kick in.

matriv · 2018-10-22T12:13:00Z

@costin as discussed, I was wrong the optimisation kicks in, it was just not tested properly, I now have a test for that. Thank you!

matriv · 2018-10-22T20:45:57Z

retest this please

Implement the functionality to translate the `field IN (value1, value2,...)` expressions to proper Lucene queries or painless script or local processors depending on the use case. The `IN` expression can be used in SELECT, WHERE and HAVING clauses. Closes: #32955

matriv · 2018-10-23T12:49:45Z

Backported to 6.x with 528e2b2

* master: (24 commits) ingest: better support for conditionals with simulate?verbose (elastic#34155) [Rollup] Job deletion should be invoked on the allocated task (elastic#34574) [DOCS] .Security index is never auto created (elastic#34589) CCR: Requires soft-deletes on the follower (elastic#34725) re-enable bwc tests (elastic#34743) Empty GetAliases authorization fix (elastic#34444) INGEST: Document Processor Conditional (elastic#33388) [CCR] Add total fetch time leader stat (elastic#34577) SQL: Support pattern against compatible indices (elastic#34718) [CCR] Auto follow pattern APIs adjustments (elastic#34518) [Test] Remove dead code from ExceptionSerializationTests (elastic#34713) A small typo in migration-assistance doc (elastic#34704) ingest: processor stats (elastic#34724) SQL: Implement IN(value1, value2, ...) expression. (elastic#34581) Tests: Add checks to GeoDistanceQueryBuilderTests (elastic#34273) INGEST: Rename Pipeline Processor Param. (elastic#34733) Core: Move IndexNameExpressionResolver to java time (elastic#34507) [DOCS] Force Merge: clarify execution and storage requirements (elastic#33882) TESTING.asciidoc fix examples using forbidden annotation (elastic#34515) SQL: Implement `CONVERT`, an alternative to `CAST` (elastic#34660) ...

Implement the functionality to translate the `field IN (value1, value2,...)` expressions to proper Lucene queries or painless script or local processors depending on the use case. The `IN` expression can be used in SELECT, WHERE and HAVING clauses. Closes: #32955

matriv added v7.0.0 :Analytics/SQL SQL querying labels Oct 18, 2018

matriv requested review from costin and astefan October 18, 2018 10:42

Fix painless script to handle ' in string values.

8c3beb3

matriv added 2 commits October 18, 2018 18:21

Fix failing test, reset static variables

527f83b

Added check for compatible data types

ba330de

astefan approved these changes Oct 18, 2018

View reviewed changes

costin requested changes Oct 18, 2018

View reviewed changes

matriv added the v6.5.0 label Oct 18, 2018

andyb-elastic mentioned this pull request Oct 18, 2018

SQL : Support 'IN' clause #34622

Closed

droberts195 and others added 8 commits October 19, 2018 13:06

Amazon: Wrap at 140 columns (elastic#34495)

27b18c4

Applies our standard column wrapping to the `discovery-ec2` and `repository-s3` plugins.

Fix line longer than 140 characters in Murmur3FieldMapper

bbf87de

Fixing line lengths in murmur3 and hdfs plugins (elastic#34603)

00b832a

Change wording for FIPS 140-2 (elastic#34471)

56cd0de

Changes wording in the FIPS 140-2 related documentation. Co-authored-by: derickson <dave@elastic.co>

$@polyfractal$

[Rollup] Add support for date histo format (elastic#34537)

5ed4a2b

Adds support for query-time formatting of the date histo keys when executing a rollup search. Closes elastic#34391

Address comments

fd3b8e0

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

4cecd50

matriv force-pushed the mt/impl-in-32955 branch from ab7c150 to 4cecd50 Compare October 19, 2018 11:08

matriv added the >enhancement label Oct 19, 2018

matriv added 2 commits October 19, 2018 16:22

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

f536820

Fix tests

27fdd06

costin approved these changes Oct 19, 2018

View reviewed changes

matriv added the >feature label Oct 19, 2018

matriv added 2 commits October 20, 2018 01:55

Address comments, remove duplicates from IN list

a73615e

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

1088454

costin reviewed Oct 21, 2018

View reviewed changes

matriv added 5 commits October 21, 2018 17:53

Address comments

6b4ddf1

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

f9dfc8f

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

0359faf

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

89abace

Added test

d1e1018

matriv force-pushed the mt/impl-in-32955 branch from 21dac9d to d1e1018 Compare October 22, 2018 12:08

matriv added 3 commits October 22, 2018 17:19

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

98e1da1

add more tests

9fc60b2

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

73b0e1d

Merge remote-tracking branch 'upstream/master' into mt/impl-in-32955

d09e171

matriv merged commit 4a8386f into elastic:master Oct 23, 2018

matriv deleted the mt/impl-in-32955 branch October 23, 2018 12:31

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		@@ -188,6 +193,16 @@ private static Failure fail(Node<?> source, String message, Object... args) {

		Set<Failure> localFailures = new LinkedHashSet<>();

		if (p instanceof Filter) {


		private static void validateInExpression(Expression e, Set<Failure> localFailures) {
		if (e instanceof In) {

SQL: Implement IN(value1, value2, ...) expression. #34581

SQL: Implement IN(value1, value2, ...) expression. #34581

Conversation

matriv commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

matriv commented Oct 18, 2018 • edited Loading

astefan commented Oct 18, 2018

astefan commented Oct 18, 2018

costin commented Oct 18, 2018

matriv commented Oct 18, 2018

astefan commented Oct 18, 2018

astefan left a comment

Choose a reason for hiding this comment

matriv commented Oct 18, 2018 • edited Loading

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin commented Oct 18, 2018

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Oct 20, 2018

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

matriv commented Oct 22, 2018

matriv commented Oct 22, 2018

matriv commented Oct 23, 2018

matriv commented Oct 18, 2018 •

edited

Loading

matriv commented Oct 18, 2018 •

edited

Loading