SQL: Fix ORDER BY YEAR() function #51562

matriv · 2020-01-28T20:10:50Z

Previously, if YEAR() was used as and ORDER BY argument without being
wrapped with another scalar (e.g. YEAR(birth_date) + 10), no script
ordering was used but instead the underlying field (e.g. birth_date)
was used instead as a performance optimisation. This works correctly if
YEAR() is the only ORDER BY arg but if further args are used as tie
breakers for the ordering wrong results are produced. This is because
2 rows with the different birth_date but on the same year are not tied
as the underlying ordering is on birth_date and not on the
YEAR(birth_date), and the following ORDER BY args are ignored.

Remove this optimisation for YEAR() to avoid incorrect results in
such cases.

As a consequence another bug is revealed: scalar functions on top
of nested fields produce scripted sorting/filtering which is not yet
supported. In such cases no error was thrown but instead all values for
such nested fields were null and were passed to the script implementing
the sorting/filtering, producing incorrect results.

Detect such cases and throw a validation exception.

Fixes: #51224

elasticmachine · 2020-01-28T20:10:52Z

Pinging @elastic/es-search (:Search/SQL)

Previously, if `YEAR()` was used as and ORDER BY argument without being wrapped with another scalar (e.g. `YEAR(birth_date) + 10`), no script ordering was used but instead the underlying field (e.g. `birth_date`) was used instead as a performance optimisation. This works correctly if YEAR() is the only ORDER BY arg but if further args are used as tie breakers for the ordering wrong results are produced. This is because 2 rows with the different `birth_date but on the same year are not tied as the underlying ordering is on `birth_date` and not on the `YEAR(birth_date)`, and the following ORDER BY args are ignored. Remove this optimisation for `YEAR()` to avoid incorrect results in such cases. Fixes: elastic#51224

astefan

One of the tests has wrong results.

astefan · 2020-01-28T22:28:31Z

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

-Chirstian            |Koblick        
-null                 |Chappelet      
-Zvonko               |Nyanchama  
+Parto                |Bamford


This test is not relevant (anymore). Can you add dep.from_date to the projections?
Why did the order change since the PR should address ONLY the case of ordering by year and something else?

The order can change because previously we would have a strict order on the whole from_date (including day and month) but now simply on the YEAR, so (apart from the YEAR) can be affected by the order of the doc entries when inserted to the index.

Ok, so how does the query dsl look like now? Previously I think it contained a "sort" : [{"from_date" : {"order" : "asc"}}] section. We don't have tests that check the generated ES query with a sort by a date field?

As discussed, since a scalar is used, it needs to be translated into a scripted sorting, which currently is not supported for nested fields.

astefan · 2020-01-28T22:30:54Z

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

-Chirstian            |Koblick        |1986           
-null                 |Chappelet      |1988           
-Zvonko               |Nyanchama      |1989     
+Parto                |Bamford        |1995


Ordering here is completely wrong. 1995, 1986, 1996.... The query has ORDER BY start where start is YEAR(dep.from_date).

Oooops! I rushed here, sorry, need to check it. Thx for catching!

matriv · 2020-01-29T12:42:02Z

Seems that with the fix, YEAR() becomes always a script, and we cannot have scripted sorting on nested fields, It's a current restriction in ES.

My suggestion is to catch this in the Verifier and not allow to use scalars on nested fields.

matriv · 2020-01-29T17:23:50Z

As discussed, added a detection to prevent usage of scalars on nested fields in ORDER BY.
I also opened an issue to enhance the error messages: #51636
as I tried to improve it but seems not so trivial.

costin

LGTM

costin · 2020-01-29T21:43:16Z

...n/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/ScalarFunction.java

-    // used if the function is monotonic and thus does not have to be computed for ordering purposes
-    // null means the script needs to be used; expression means the field/expression to be used instead
+    // Every function needs to be translated to a script
+    // which will be used for the ordering


I think the method can be removed entirely - is it used anywhere else?

costin · 2020-01-29T21:44:13Z

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

        List<FieldAttribute> nested = new ArrayList<>();
-        Consumer<FieldAttribute> match = fa -> {
+        Consumer<FieldAttribute> match = (fa) -> {


why (fa)?

costin · 2020-01-29T21:46:41Z

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

            if (fa.isNested()) {
                nested.add(fa);
            }
        };

        // nested fields shouldn't be used in aggregates or having (yet)
-        p.forEachDown(a -> a.groupings().forEach(agg -> agg.forEachUp(match, FieldAttribute.class)), Aggregate.class);
+        p.forEachDown(a -> a.groupings().forEach(agg -> agg.forEachUp(e ->


Small nit - this type of call seems to be used 3 times (last time twice) - maybe could be extracted into a small method to avoid repetition.
Maybe check if it's used anywhere else so even more of a reuse...

costin · 2020-01-29T21:47:32Z

Please add also a section in the docs mentioning that scalar functions cannot be used in ordering on nested fields. Maybe even filtering...

astefan

LGTM
But please, add a test to QueryTranslatorTests for ORDER BY YEAR() without GROUPing BY YEAR(). This PR proves that a small enough change (the overriden orderBy() method removal in Year) can change significantly the generated ES query.

astefan

Left few comments.

astefan · 2020-01-30T11:48:01Z

docs/reference/sql/limitations.asciidoc

+
+[source, sql]
+--------------------------------------------------
+SELECT * FROM test_emp WHERE length(dep.dep_name.keyword) > 5;


Capital letters for length?

astefan · 2020-01-30T11:56:24Z

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

            if (fa.isNested()) {
                nested.add(fa);
            }
        };
+        Consumer<Expression> checkForNested = e ->
+                attributeRefs.getOrDefault(e, e).forEachUp(matchNested, FieldAttribute.class);
+        Consumer<ScalarFunction> checkForNestedInFcuntion =  f -> f.arguments().forEach(


Typo in the variable name: checkForNestedInFcuntion .

astefan · 2020-01-30T12:00:31Z

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

-                            if (sf.orderBy() != null) {
-                                Expression orderBy = sf.orderBy();
-                                if (orderBy instanceof NamedExpression) {
-                                    orderBy = qContainer.aliases().getOrDefault(orderBy, orderBy);
-                                    qContainer = qContainer
-                                            .addSort(new AttributeSort(((NamedExpression) orderBy).toAttribute(), direction, missing));
-                                } else if (orderBy.foldable() == false) {
-                                    // ignore constant
-                                    throw new PlanningException("does not know how to order by expression {}", orderBy);
-                                }
-                            } else {


I don't understand why this logic is removed. We removed the faulty support for sorting on nested fields that needs scripting, but why is this block of code associated with nested fields?

This code was especially for Scalar Functions in case the orderBy() didn't return null -> no script creation, instead use the underlying field fo the ordering. The only use case of this was the YEAR() function.

Previously, if YEAR() was used as and ORDER BY argument without being wrapped with another scalar (e.g. YEAR(birth_date) + 10), no script ordering was used but instead the underlying field (e.g. birth_date) was used instead as a performance optimisation. This works correctly if YEAR() is the only ORDER BY arg but if further args are used as tie breakers for the ordering wrong results are produced. This is because 2 rows with the different birth_date but on the same year are not tied as the underlying ordering is on birth_date and not on the YEAR(birth_date), and the following ORDER BY args are ignored. Remove this optimisation for YEAR() to avoid incorrect results in such cases. As a consequence another bug is revealed: scalar functions on top of nested fields produce scripted sorting/filtering which is not yet supported. In such cases no error was thrown but instead all values for such nested fields were null and were passed to the script implementing the sorting/filtering, producing incorrect results. Detect such cases and throw a validation exception. Fixes: #51224 (cherry picked from commit f41efd6)

matriv added >bug :Analytics/SQL SQL querying v8.0.0 v6.8.7 v7.7.0 v7.6.1 v7.5.3 labels Jan 28, 2020

matriv requested review from costin, astefan and bpintea January 28, 2020 20:10

matriv added 2 commits January 28, 2020 21:47

adapt test

94251c9

matriv force-pushed the fix-51224 branch from b3f7fbb to 94251c9 Compare January 28, 2020 20:55

astefan requested changes Jan 28, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-51224

2aa1aee

Detect and prevent scalars on nested for ORDER BY

1875e76

matriv requested a review from astefan January 29, 2020 17:22

costin approved these changes Jan 29, 2020

View reviewed changes

astefan approved these changes Jan 30, 2020

View reviewed changes

Address comments - disallow also in WHERE

801ca3e

matriv requested a review from astefan January 30, 2020 11:47

Merge remote-tracking branch 'upstream/master' into fix-51224

e50c2fd

astefan reviewed Jan 30, 2020

View reviewed changes

address comments - typos/formatting

88553db

matriv removed the v7.5.3 label Jan 30, 2020

matriv merged commit f41efd6 into elastic:master Jan 30, 2020

matriv deleted the fix-51224 branch January 30, 2020 13:48

$@polyfractal$ polyfractal added v7.6.0 and removed v7.6.1 labels Feb 7, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

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 ORDER BY YEAR() function #51562

SQL: Fix ORDER BY YEAR() function #51562

matriv commented Jan 28, 2020 •

edited

Loading

elasticmachine commented Jan 28, 2020

astefan left a comment

astefan Jan 28, 2020

matriv Jan 29, 2020

astefan Jan 29, 2020

matriv Jan 29, 2020

astefan Jan 28, 2020

matriv Jan 29, 2020

matriv commented Jan 29, 2020 •

edited

Loading

matriv commented Jan 29, 2020

costin left a comment

costin Jan 29, 2020

costin Jan 29, 2020

costin Jan 29, 2020

costin commented Jan 29, 2020

astefan left a comment

astefan left a comment

astefan Jan 30, 2020

astefan Jan 30, 2020

astefan Jan 30, 2020

matriv Jan 30, 2020

SQL: Fix ORDER BY YEAR() function #51562

SQL: Fix ORDER BY YEAR() function #51562

Conversation

matriv commented Jan 28, 2020 • edited Loading

elasticmachine commented Jan 28, 2020

astefan 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

matriv commented Jan 29, 2020 • edited Loading

matriv commented Jan 29, 2020

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

costin commented Jan 29, 2020

astefan left a comment

Choose a reason for hiding this comment

astefan 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 Jan 28, 2020 •

edited

Loading

matriv commented Jan 29, 2020 •

edited

Loading