QL: Turn eager query translations lazy #69205

palesz · 2021-02-18T17:34:46Z

Before the ExpressionTranslators did some unnecessary
Expression -> Query translations, where the result queries were thrown
away by later translations (by the wrapFunctionQuery).

This change adds laziness, so translations don't execute unnecessarily.

Follows #68783 and #68788

Note: #68788 (comment) requested more SqlTranslatorHandler tests, but the 9fc11a5 commit shows that the code is unreachable. Can only be reached when GeoDistanceQuery is generated and the test that generates a GeoDistanceQuery does not hit the line, so decided to remove it.

Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows elastic#68783

* SqlTranslatorHandler dead code removal * Pushed the common `wrapFunctionQuery` implementation into the TranslatorHandler

palesz · 2021-02-19T16:03:35Z

@elasticmachine run elasticsearch-ci/2

elasticmachine · 2021-02-19T17:05:20Z

Pinging @elastic/es-ql (Team:QL)

costin

LGTM - however I'd like a few more tests. The changes are small but somewhat subtle and since some code is removed and as you pointed, unused, it would help to have more tests on the front (in this case geo) to avoid ending in a similar situation in the future as we're blind on that front.

costin · 2021-02-20T08:17:27Z

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

@@ -1334,6 +1334,7 @@ public void testTranslateStDistanceToQuery() {
        assertEquals(20.0, gq.lat(), 0.00001);
        assertEquals(10.0, gq.lon(), 0.00001);
        assertEquals(25.0, gq.distance(), 0.00001);
+        optimizeAndPlan(p);


? - there are no extra assertions after this call.

The optimizeAndPlan(p) was introduced as part of the 9fc11a5 commit to make sure we exercise the SqlTranslationHandler (the translate() does not do that) and check if it will produce a RuntimeException. Added an extra check on the generated query.

Added a test for the Verifier to capture the current behaviour: the WHERE ST_Distance(nested.point, ...) < X is not even allowed today (WHERE isn't (yet) compatible with scalar functions on nested fields).

costin · 2021-02-20T08:29:19Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java


+        private static Query translate(IsNotNull isNotNull, TranslatorHandler handler) {


Use a different name to avoid confusion, it's counter intuitive for doTranslate to call translate, I would have expected it to be the other way around.
Maybe createQuery or internalTranslate

Might be a tad confusing, but this "doTranslate -> translate" pattern seems to be a pre-existing one.

As @bpintea pointed out the doTranslate / translate methods are pre-existing. I would do the renames in a separate PR across the all the translators and keep this PR focused only on laziness.

Also fine for me, to address this later!

costin · 2021-02-20T08:32:52Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java

@@ -265,7 +271,7 @@ public static void checkBinaryComparison(BinaryComparison bc) {

        public static Query doTranslate(BinaryComparison bc, TranslatorHandler handler) {
            checkBinaryComparison(bc);
-            return handler.wrapFunctionQuery(bc, bc.left(), translate(bc, handler));
+            return handler.wrapFunctionQuery(bc, bc.left(), () -> translate(bc, handler));


Same here even though translate existed before it should be renamed.

bpintea

LGTM

bpintea · 2021-02-22T11:13:01Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/ExpressionTranslators.java


+        private static Query translate(IsNotNull isNotNull, TranslatorHandler handler) {


Might be a tad confusing, but this "doTranslate -> translate" pattern seems to be a pre-existing one.

bpintea · 2021-02-22T11:32:18Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/planner/TranslatorHandler.java

+            return ExpressionTranslator.wrapIfNested(querySupplier.get(), field);
+        }
+        return new ScriptQuery(sf.source(), sf.asScript());
+    }

    String nameOf(Expression e);


Loosely coupled to this refactoring, but: couldn't nameOf() also be offered a static implementation here? All implementations could actually have it as a static method and SQL's (whose implementation lives actually in QueryTranslator) only treats the DateTimeFunction case additionally, compared to QL's.
So a bit more duplication could be stripped away.

astefan

Removing "dead" code without any good reason doesn't sound great from my side. The code has been added there with a good reason, hopefully; removing it should have an equally good reason, beside being "dead". I'd like a bit more investigation put into this issue to try and track down any refactoring (I'm mainly looking here at the split from one SQL project to QL/SQL/EQL) that could have led to this "dead" code now.

Maybe test the same 9fc11a5 commit in an older branch (before ql/sql/eql refactoring), or in the same commit that added the GEO part to SQL project. Maybe the code was added for no good reason since the beginning, maybe its utility faded with subsequent refactoring, but I think it's important to know why this code is dead now.

palesz · 2021-03-01T18:13:34Z

Removing "dead" code without any good reason doesn't sound great from my side. The code has been added there with a good reason, hopefully; removing it should have an equally good reason, beside being "dead". I'd like a bit more investigation put into this issue to try and track down any refactoring (I'm mainly looking here at the split from one SQL project to QL/SQL/EQL) that could have led to this "dead" code now.

Maybe test the same 9fc11a5 commit in an older branch (before ql/sql/eql refactoring), or in the same commit that added the GEO part to SQL project. Maybe the code was added for no good reason since the beginning, maybe its utility faded with subsequent refactoring, but I think it's important to know why this code is dead now.

@astefan See below.

Since Jan, 2020 the following change does not allow scalar functions on nested fields, which means that the queries where the ST_Distance -> GeoDistanceQuery optimization could act on nested fields, don't even get to the QueryTranslator, rather fail early with WHERE isn't (yet) compatible with scalar functions on nested fields. Note: the translation still works as expected on non-nested fields.
Even if the nested queries would pass the verifier, since the SQL -> QL/SQL refactor the optional wrapping into nested in case of nested fields is not called. As you can see, when I removed the dead code, I added back this optional potential nested query wrapped to the place where the optimized query is generated, so in case we remove the verifier checks a reasonable query translation will happen for the nested fields.

I'd open a separate issue if we decide to make this particular optimization work on nested fields again.

matriv · 2021-03-02T10:07:59Z

@palesz

Sounds good to me!

astefan

LGTM. Thank you @palesz

matriv

LGTM, delayed sorry :)

Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows elastic#68783 and elastic#68788

Before the `ExpressionTranslators` did some unnecessary `Expression` -> `Query` translations, where the result queries were thrown away by later translations (by the `wrapFunctionQuery`). This change adds laziness, so translations don't execute unnecessarily. Follows #68783 and #68788

Andras Palinkas added 5 commits February 18, 2021 12:29

Tests that code is dead within the SqlTranslatorHandler

9fc11a5

Merge remote-tracking branch 'origin/master' into lazy-translators

560c6db

TranslatorHandler changes

4be1ac0

* SqlTranslatorHandler dead code removal * Pushed the common `wrapFunctionQuery` implementation into the TranslatorHandler

Merge remote-tracking branch 'origin/master' into lazy-translators

d80820e

palesz added :Analytics/SQL SQL querying >refactoring Team:QL (Deprecated) Meta label for query languages team v7.13.0 v8.0.0 labels Feb 19, 2021

palesz requested review from costin, bpintea, astefan and matriv February 19, 2021 17:05

palesz marked this pull request as ready for review February 19, 2021 17:05

costin approved these changes Feb 20, 2021

View reviewed changes

bpintea reviewed Feb 22, 2021

View reviewed changes

astefan reviewed Feb 23, 2021

View reviewed changes

Andras Palinkas added 4 commits March 1, 2021 12:35

Test ST_Distance calls on nested fields

e3258a2

Merge remote-tracking branch 'origin/master' into lazy-translators

ff9d66f

Remove the non-required field from mapping

51460a4

Merge remote-tracking branch 'origin/master' into lazy-translators

9c786d3

astefan approved these changes Mar 2, 2021

View reviewed changes

palesz merged commit 2dbd59b into elastic:master Mar 2, 2021

matriv reviewed Mar 2, 2021

View reviewed changes

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

QL: Turn eager query translations lazy #69205

QL: Turn eager query translations lazy #69205

palesz commented Feb 18, 2021 •

edited

Loading

palesz commented Feb 19, 2021

elasticmachine commented Feb 19, 2021

costin left a comment

costin Feb 20, 2021

palesz Mar 1, 2021

costin Feb 20, 2021

matriv Feb 22, 2021

bpintea Feb 22, 2021

palesz Feb 22, 2021

matriv Feb 22, 2021

costin Feb 20, 2021

bpintea left a comment

bpintea Feb 22, 2021

bpintea Feb 22, 2021

astefan left a comment

palesz commented Mar 1, 2021

matriv commented Mar 2, 2021

astefan left a comment

matriv left a comment


		private static Query translate(IsNotNull isNotNull, TranslatorHandler handler) {

QL: Turn eager query translations lazy #69205

QL: Turn eager query translations lazy #69205

Conversation

palesz commented Feb 18, 2021 • edited Loading

palesz commented Feb 19, 2021

elasticmachine commented Feb 19, 2021

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

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

palesz commented Mar 1, 2021

matriv commented Mar 2, 2021

astefan left a comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

palesz commented Feb 18, 2021 •

edited

Loading