SQL: Fix issue with IN not resolving to underlying keyword field #38440

matriv · 2019-02-05T16:04:02Z

Add resolution to the exact keyword field (if exists) for text fields.
Add proper verification and error message if underlying keyword
doesn't exist.
Move check for field attribute in the comparison list to the
resolveType() method of IN.

elasticmachine · 2019-02-05T16:04:05Z

Pinging @elastic/es-search

Add resolution to the exact keyword field (if exists) for text fields. Add proper verification and error message if underlying keyword doesn't exist. Move check for field attribute in the comparison list to the `resolveType()` method of `IN`. Fixes: elastic#38424

astefan

LGTM overall. Left two comments.

astefan · 2019-02-05T16:28:43Z

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

+                        if (fa.isInexact()) {
+                            name = fa.exactAttribute().name();
+                        }
+                        q = new TermsQuery(in.source(), name, in.list());


My personal preference is for q = new TermsQuery(in.source(), fa.isInexact() ? fa.exactAttribute().name() : fa.name(), in.list());.

astefan · 2019-02-05T16:43:16Z

...l/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java

+            try {
+                ((FieldAttribute) value).exactAttribute();
+            } catch (MappingException ex) {
+                return new TypeResolution(format(null, "[{}] cannot operate on first argument field of data type [{}]",


I am not sure this is the correct error message here. Looking at the test here I think it will be confusing for users as to why they get that error message. Meaning, text is a text field, and the query is WHERE text IN ('foo', 'bar') (everything is text) but the error message says one cannot use text. I think it would help users if the error message would be a bit special for text: maybe say between brackets "needs to be of keyword type" or something similar - give a hint to the user for the reason of that error.

I used the same message in First/Last.
The MappingException is thrown in this case (text with no underlying keyword) but since it's a generic exception I was "afraid" to add a specific message for the case. (More like future proof driven).
@costin what do you think?

costin

Looks good - left some comments mainly related to style.
I wonder if we should consider supporting text fields as well...

costin · 2019-02-05T16:09:30Z

...l/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java

@@ -105,6 +108,26 @@ protected Pipe makePipe() {
        return new InPipe(source(), this, children().stream().map(Expressions::pipe).collect(Collectors.toList()));
    }

+    @Override
+    protected TypeResolution resolveType() {
+        if (value instanceof FieldAttribute) {


The underlying error message should bubble up since it's important. It might make sense for these cases to have a non-exception method as the MappingException itself seems to heavy for the analyzer.

Created an issue to improve this (the exception part) globally: #38501

costin · 2019-02-05T19:42:10Z

...l/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/comparison/In.java

+            }
+        }
+
+        Optional<Expression> firstNotFoldable = list.stream().filter(expression -> !expression.foldable()).findFirst();


I think this is more readable through a basic for loop and early return

for (Expression ex : list) { if(ex.foldable() == false) { return new TypeResolution... } }

About the same amount of lines, straight-forward and faster.

matriv · 2019-02-06T10:24:40Z

@costin addressed comments.

matriv · 2019-02-06T10:45:22Z

@elasticmachine run elasticsearch-ci/default-distro

astefan · 2019-02-06T11:06:26Z

.../src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

@@ -410,7 +410,8 @@ public void testInWithFieldInListOfValues() {
    }

    public void testInOnFieldTextWithNoKeyword() {
-        assertEquals("1:26: [IN] cannot operate on first argument field of data type [text]",
+        assertEquals("1:26: [IN] cannot operate on field of data type [text]: " +
+            "No keyword/multi-field defined exact matches for [text]; define one or use MATCH/QUERY instead",


I like this one.

costin

LGTM

) - Add resolution to the exact keyword field (if exists) for text fields. - Add proper verification and error message if underlying keyword doesn'texist. - Move check for field attribute in the comparison list to the `resolveType()` method of `IN`. Fixes: #38424

matriv · 2019-02-06T15:27:57Z

Backported to 7.x with f96bd2a
to 7.0 with 5e798c1
to 6.7 with f5e4de7
to 6.6 with 841eb71

* master: ML: update set_upgrade_mode, add logging (elastic#38372) bad formatted JSON object (elastic#38515) (elastic#38525) Fix HistoryIntegrationTests timestamp comparsion (elastic#38505) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Fix the clock resolution to millis in ScheduledEventTests (elastic#38506)

* 6.6: (121 commits) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38608) fix dissect doc "ip" --> "clientip" (elastic#38512) bad formatted JSON object (elastic#38515) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Update ilm-api.asciidoc, point to REMOVE policy (elastic#38235) Backport changes to the release notes script. (elastic#38347) Change the milliseconds precision to 3 digits for intervals. (elastic#38297) SecuritySettingsSource license.self_generated: trial (elastic#38233) (elastic#38398) Fix IndexAuditTrail rolling upgrade on rollover edge 2 (elastic#38286) (elastic#38381) Cleanup construction of interceptors (elastic#38388) Skip unsupported languages for tests (elastic#38328) (elastic#38385) [ILM][TEST] increase assertBusy timeout (elastic#36864) (elastic#38354) Docs: Drop inline callout from scroll example (elastic#38340) (elastic#38365) Preserve ILM operation mode when creating new lifecycles (elastic#38134) (elastic#38230) [ML] Add explanation so far to file structure finder exceptions (elastic#38337) ML: Fix error race condition on stop _all datafeeds and close _all jobs (elastic#38113) (elastic#38211) (elastic#38222) SQL: Generate relevant error message when grouping functions are not used in GROUP BY (elastic#38017) Fix NPE in Logfile Audit Filter (elastic#38120) (elastic#38273) Enable trace log in FollowerFailOverIT (elastic#38148) Replace awaitBusy with assertBusy in atLeastDocsIndexed (elastic#38190) ...

* master: (27 commits) Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) Mute failing WatchStatusIntegrationTests (elastic#38621) Mute failing ApiKeyIntegTests (elastic#38614) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509) Add 7.1 and 8.0 version constants to master (elastic#38514) ML: update set_upgrade_mode, add logging (elastic#38372) bad formatted JSON object (elastic#38515) (elastic#38525) Fix HistoryIntegrationTests timestamp comparsion (elastic#38505) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Fix the clock resolution to millis in ScheduledEventTests (elastic#38506) Enable BWC after backport recovering leases (elastic#38485) Collapse retention lease integration tests (elastic#38483) TransportVerifyShardBeforeCloseAction should force a flush (elastic#38401) ...

matriv added >bug v7.0.0 :Analytics/SQL SQL querying v6.7.0 v6.6.1 labels Feb 5, 2019

matriv requested review from costin and astefan February 5, 2019 16:04

matriv force-pushed the mt/fix-38437 branch from 4e43664 to 9671e94 Compare February 5, 2019 16:13

astefan approved these changes Feb 5, 2019

View reviewed changes

costin reviewed Feb 5, 2019

View reviewed changes

Address comments

b0729dd

astefan reviewed Feb 6, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into mt/fix-38437

9b9dc97

costin approved these changes Feb 6, 2019

View reviewed changes

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

matriv added the v7.0.0 label Feb 6, 2019

matriv merged commit 861eee7 into elastic:master Feb 6, 2019

matriv deleted the mt/fix-38437 branch February 6, 2019 14:19

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

colings86 added the v7.2.0 label Jun 18, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Fix issue with IN not resolving to underlying keyword field #38440

SQL: Fix issue with IN not resolving to underlying keyword field #38440

matriv commented Feb 5, 2019 •

edited

Loading

elasticmachine commented Feb 5, 2019

astefan left a comment

astefan Feb 5, 2019

astefan Feb 5, 2019 •

edited

Loading

matriv Feb 5, 2019

costin left a comment •

edited

Loading

costin Feb 5, 2019

matriv Feb 6, 2019 •

edited

Loading

costin Feb 5, 2019

matriv commented Feb 6, 2019

matriv commented Feb 6, 2019

astefan Feb 6, 2019

costin left a comment

matriv commented Feb 6, 2019

SQL: Fix issue with IN not resolving to underlying keyword field #38440

SQL: Fix issue with IN not resolving to underlying keyword field #38440

Conversation

matriv commented Feb 5, 2019 • edited Loading

elasticmachine commented Feb 5, 2019

astefan left a comment

Choose a reason for hiding this comment

astefan Feb 5, 2019

Choose a reason for hiding this comment

astefan Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

matriv Feb 5, 2019

Choose a reason for hiding this comment

costin left a comment • edited Loading

Choose a reason for hiding this comment

costin Feb 5, 2019

Choose a reason for hiding this comment

matriv Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

costin Feb 5, 2019

Choose a reason for hiding this comment

matriv commented Feb 6, 2019

matriv commented Feb 6, 2019

astefan Feb 6, 2019

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

matriv commented Feb 6, 2019

matriv commented Feb 5, 2019 •

edited

Loading

astefan Feb 5, 2019 •

edited

Loading

costin left a comment •

edited

Loading

matriv Feb 6, 2019 •

edited

Loading