SQL: Enhance checks for inexact fields by matriv · Pull Request #39427 · elastic/elasticsearch

matriv · 2019-02-27T00:04:09Z

For functions: move checks for text fields without underlying keyword
fields or with many of them (ambiguity) to the type resolution stage.

For Order By/Group By: move checks to the Verifier to catch early
before QueryTranslator or execution.

Fixes: #38501

For functions: move checks for `text` fields without underlying `keyword` fields or with many of them (ambiguity) to the type resolution stage. For Order By/Group By: move checks to the `Verifier` to catch early before `QueryTranslator` or execution. Fixes: elastic#38501

elasticmachine · 2019-02-27T00:04:10Z

Pinging @elastic/es-search

matriv · 2019-02-27T10:00:40Z

Also fixes: #35203

matriv · 2019-02-27T10:03:51Z

@costin @astefan As this PR fixes also an issue for group by and order by, do you think I should extract this fix separately as it's done here: #39443, to backport only the fix, or should I backport this whole PR to earlier versions?

Asking because this extraction requires some more work.

costin

Thanks for looking into this. Left a few comments, mainly around naming.

costin · 2019-02-27T13:08:52Z

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

+import static org.elasticsearch.xpack.sql.expression.Expressions.name;
+import static org.elasticsearch.xpack.sql.type.DataType.BOOLEAN;
+
+public final class TypeResolutionUtils {


I would prefer using the plural instead of Utils - TypeResolutions. Utils has been used where the plural already existed in the rest of the codebase to avoid name clashes.

The plural just sounded to me a bit weird, but I'll change.

costin · 2019-02-27T13:12:50Z

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

+
+    private TypeResolutionUtils() {}
+
+    public static TypeResolution typeMustBeBoolean(Expression e, String operationName, ParamOrdinal paramOrd) {


Additionally, how about simplifying the method names from "typeMustBeNumeric" to "isNumeric" similar to the assertion in JUnit? It also add consistency ("expressionMustBe", "typeMustBe...").

costin · 2019-02-27T13:15:09Z

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

+        return TypeResolution.TYPE_RESOLVED;
+    }
+
+    public static TypeResolution expressionMustBeTableColumn(Expression e, String operationName, ParamOrdinal paramOrd) {


constant/table column is a bit misleading - I would just use isFoldable/notFoldable since it's a terminology already used through-out the code.

costin · 2019-02-27T13:19:04Z

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

+
+    @Override
+    public Tuple<Boolean, String> hasExact() {
+        return PROCESS_EXACT_FIELD.apply(findExact());


What's the reason behind using the closure?

The findExact() returns also the exact field (used by getExact()) so I wanted to convert the field == null ? false : true and chose a Function to be somehow more elegant.

I got that but why not use the code directly? The closure is not passed anywhere...

Tuple<EsField, String> tuple = findExact(); return tuple.v1() == null ? new Exact(false, tuple.v2()) : new Exact(true, null)

costin · 2019-02-27T13:22:05Z

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

        assertEquals("value", tq.value());
    }

-    public void testTermEqualityAnalyzerAmbiguous() {


Is this tested somewhere else?

Yes, here: https://github.com/elastic/elasticsearch/pull/39427/files#diff-334bd2f8c33b60789817181fcef4ca52R331 with 2 methods now.

costin · 2019-02-27T13:25:24Z

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

+     * {False, String} if this field doesn't have an underlying exact field, the v2 of the tuple is a string containing
+     * an error message explaining why that is.
+     */
+    public Tuple<Boolean, String> hasExact() {


I think having both isExact and hasExact is confusing - is the former still used anywhere? I opt for refactoring isExact to return the tuple instead (which can be promoted to a dedicated inner class such (Exact) to make its use easier and its intent clearer).

I agree with the confusing isExact but it's used externally here: https://github.com/elastic/elasticsearch/pull/39427/files#diff-1059b5208ef7d10a0fdc89b5dad914edR90 and has slightly different semantics than hasExact.

hasExact returns <TRUE, null> if it's exact, or has an exact underlying field, so we don't know which is the case.

But if we introduce a class Exact to replace the the Tuple we can include this info there and remove the confusing isExact() method. Thx!

costin · 2019-02-27T14:07:19Z

@costin @astefan As this PR fixes also an issue for group by and order by, do you think I should extract this fix separately as it's done here: #39443, to backport only the fix, or should I backport this whole PR to earlier versions?

Asking because this extraction requires some more work.

Whatever it's easier - I think the PR itself is not complicated but is made big by the amount of files that it touches. I'm fine with backporting the whole PR.

matriv · 2019-02-27T16:03:22Z

@costin thanks for the comments! Pushed a commit to address them.

astefan

LGTM. Nice cleanup. Left few minor comments.

astefan · 2019-02-27T15:52:03Z

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

    }

+    public void testGroupByOnInexact() {
+        assertEquals("1:36: Grouping field of data type [text] for grouping; " +


I am not sure I understand the first sentence in this error message, it has no predicate. Can you try a more descriptive message?

ah, sorry I mixed something up there, will fix.

thx for catching!

astefan · 2019-02-27T15:56:15Z

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


+    @Override
+    protected TypeResolution resolveType() {
+        return typeMustBeExact(child, "ORDER BY cannot be applied to field of data type[{}]: {}");


White space between type and [{}]: ...to field of data type [{}]: {}.

astefan · 2019-02-27T15:59:37Z

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

    }

+    public void testInvalidTypeForStringFunction_WithOneArgNumeric() {
+        assertEquals("1:8: argument of [CHAR('foo')] must be [integer], found value ['foo'] type [keyword]",


How does the error message look like for something like SELECT ASCII(CHAR('foo'))? (nested functions)

Added a test: cda66ca#diff-334bd2f8c33b60789817181fcef4ca52R457

Good thinking! But seems fine since we use sourceText() on the offending function and it's not showing the whole expression that can be complex and result in confusion for the user.

astefan · 2019-02-27T16:06:19Z

Also, shouldn't this one go to 7.0.x as well?

matriv · 2019-02-27T16:21:10Z

@astefan yes, it will actually be backported all the way to 6.6.2 as it also fixes the bug mentioned for Order By and Group By.

costin

LGTM.

For functions: move checks for `text` fields without underlying `keyword` fields or with many of them (ambiguity) to the type resolution stage. For Order By/Group By: move checks to the `Verifier` to catch early before `QueryTranslator` or execution. Closes: #38501 Fixes: #35203

matriv · 2019-03-01T12:12:11Z

Backported to 7.x with 9fb2f67
to 7.0 with 2c39bb3
to 6.7 with bfc6e22
tp 6.6 with 9c60367

MIN/MAX on strings are supported and are implemented with TopAggs FIRST/LAST respectively, but they cannot operate on `text` fields without underlying `keyword` fields => inexact. Follows: elastic#39427

MIN/MAX on strings are supported and are implemented with TopAggs FIRST/LAST respectively, but they cannot operate on `text` fields without underlying `keyword` fields => inexact. Follows: #39427

matriv added >enhancement :Analytics/SQL SQL querying v8.0.0 v7.2.0 labels Feb 27, 2019

matriv requested review from astefan and costin February 27, 2019 00:04

matriv added the >bug label Feb 27, 2019

costin reviewed Feb 27, 2019

View reviewed changes

matriv added 2 commits February 27, 2019 16:07

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

12d705a

Address comments

0ea7c4c

astefan approved these changes Feb 27, 2019

View reviewed changes

matriv added v7.0.0 v6.7.0 v6.6.2 labels Feb 27, 2019

Addressed comments

cda66ca

costin approved these changes Feb 28, 2019

View reviewed changes

matriv merged commit 10ab391 into elastic:master Feb 28, 2019

matriv deleted the mt/fix-38501 branch February 28, 2019 23:10

matriv mentioned this pull request Mar 1, 2019

SQL: Don't allow inexact fields for MIN/MAX #39563

Merged

michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

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


		private TypeResolutionUtils() {}

		public static TypeResolution typeMustBeBoolean(Expression e, String operationName, ParamOrdinal paramOrd) {

Conversation

matriv commented Feb 27, 2019

Uh oh!

elasticmachine commented Feb 27, 2019

Uh oh!

matriv commented Feb 27, 2019

Uh oh!

matriv commented Feb 27, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin commented Feb 27, 2019

Uh oh!

matriv commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Feb 27, 2019

Uh oh!

matriv commented Feb 27, 2019

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

costin Feb 28, 2019 •

edited

Loading

matriv commented Feb 27, 2019 •

edited

Loading