SQL: Fix result column names for arithmetic functions #33500

matriv · 2018-09-07T09:52:08Z

Previously, When an arithmetic function is applied on a
table column in the SELECT clause the name of the result
column name contained weird characters used internally when
processing the SQL statement. E.g.:

SELECT CHAR(emp_no % 10000) FROM "test_emp"

returned:

CHAR((emp_no{f}#14) % 10000))

as the column name instead of:

CHAR((emp_no) % 10000))

Fixes: #31869

costin

Left a comment, otherwise LGTM

costin · 2018-09-07T09:59:35Z

...va/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java

@@ -65,7 +66,7 @@ protected ProcessorDefinition makeProcessorDefinition() {
    public String name() {
        StringBuilder sb = new StringBuilder();
        sb.append("(");
-        sb.append(left());
+        appendFunctionArg(left(), sb);


No need for the appendFunctionArg - use Expressions.name instead which is more generic.

The instanceof check must be kept, because, currently, Literal->LeafExpression->Expression so doesn’t inherit from NamedExpression and the Expressions.name(<literal>) results to the String Literal.

Changed the code in Expressions.name() to include the case of Literal and replaced the appendFunctionArg() call with Expressions.name().

Thanks!

elasticmachine · 2018-09-07T10:04:17Z

Pinging @elastic/es-search-aggs

astefan

LGTM

Previously, When an arithmetic function is applied on a table column in the `SELECT` clause the name of the result column name contained weird characters used internally when processing the SQL statement. E.g.: SELECT CHAR(emp_no % 10000) FROM "test_emp" returned: CHAR((emp_no{f}elastic#14) % 10000)) as the column name instead of: CHAR((emp_no) % 10000)) Fixes: elastic#31869

nik9000 · 2018-09-07T12:43:57Z

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

@@ -82,7 +82,13 @@ public static AttributeSet references(List<? extends Expression> exps) {
    }

    public static String name(Expression e) {
-        return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName();
+        if (e instanceof NamedExpression) {


I feel like we'd be better off being more OO here and adding a colunmName function to Expression or something like that. The fix is fine with me as it stands but I think something like that would make a good follow up change.

This is a good example of code exposing conceptual leaks. You are right that this needs OO-ing. There's already a class for that NamedExpression is just that Literals are not yet one. Which is the culprit - I'll raise an issue to chase that down and make sure it sits well in the overall structure.

Raised #33523

Add more integ tests for complex functions calls in `SELECT` clause, to test both the correct column names returned but also the evaluation itself. Fix an issue that causes a ClassCastException to be thrown when using functions where both arguments are literals.

matriv · 2018-09-07T15:54:56Z

Please take another look as a new commit is added that adds more tests and fixes a newly discovered issue.

nik9000

Still better than it was so I'm good with it.

costin · 2018-09-10T06:21:51Z

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

@@ -68,6 +69,9 @@ protected ScriptTemplate asScript(Expression exp) {
            if (attr instanceof AggregateFunctionAttribute) {
                return asScriptFrom((AggregateFunctionAttribute) attr);
            }
+            if (attr instanceof LiteralAttribute) {


Good catch.
this is likely the cause of #33461.
I'm curious though why the resulting literal is not caught by the if (exp.foldable()) above, before getting the attribute from the expression?

Because at that point the literal expression is an Alias which is not foldable and then becomes a LiteralAttribute from the Expressions.attribute(exp) call.

elastic#33461

Previously, when an arithmetic function got applied on a table column in the `SELECT` clause, the name of the result column contained weird characters used internally when processing the SQL statement e.g.: SELECT CHAR(emp_no % 10000) FROM "test_emp" returned: CHAR((emp_no{f}#14) % 10000)) as the column name instead of: CHAR((emp_no) % 10000)) Also, fix an issue that causes a ClassCastException to be thrown when using functions where both arguments are literals. Closes #31869 Closes #33461

matriv · 2018-09-10T20:45:46Z

Backported to 6.x. -> d28c23a

* master: Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) Fix typos (elastic#33499) [CCR] Delay auto follow license check (elastic#33557) [CCR] Add create_follow_index privilege (elastic#33559) Strengthen FilterRoutingTests (elastic#33149) Correctly handle PKCS#11 tokens for system keystore (elastic#33460) Remove some duplicate request conversion methods. (elastic#33538)

* master: (91 commits) Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568) Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) ...

matriv requested review from costin and astefan September 7, 2018 09:52

matriv force-pushed the mt/fix-31869 branch from fec8949 to 70e6073 Compare September 7, 2018 09:56

costin approved these changes Sep 7, 2018

View reviewed changes

costin added >bug v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Sep 7, 2018

astefan approved these changes Sep 7, 2018

View reviewed changes

matriv added 2 commits September 7, 2018 13:06

fixup! SQL: Fix result column names for arithmetic funcs

032686c

matriv force-pushed the mt/fix-31869 branch from 70e6073 to 032686c Compare September 7, 2018 11:07

nik9000 approved these changes Sep 7, 2018

View reviewed changes

matriv added 3 commits September 7, 2018 15:56

fixup! fixup! SQL: Fix result column names for arithmetic funcs

c413094

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

46c1582

nik9000 reviewed Sep 7, 2018

View reviewed changes

costin approved these changes Sep 10, 2018

View reviewed changes

matriv added 3 commits September 10, 2018 11:23

!fixup Add test with negative literal.

8692616

elastic#33461

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

55fea13

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

23eb6d2

matriv merged commit f441bb8 into elastic:master Sep 10, 2018

matriv deleted the mt/fix-31869 branch September 25, 2018 13:21

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

jimczi removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Fix result column names for arithmetic functions #33500

SQL: Fix result column names for arithmetic functions #33500

matriv commented Sep 7, 2018

costin left a comment

costin Sep 7, 2018

matriv Sep 7, 2018 •

edited

Loading

matriv Sep 7, 2018

elasticmachine commented Sep 7, 2018

astefan left a comment

nik9000 Sep 7, 2018

costin Sep 7, 2018

costin Sep 7, 2018

matriv commented Sep 7, 2018

nik9000 left a comment

costin Sep 10, 2018

matriv Sep 10, 2018

matriv commented Sep 10, 2018

SQL: Fix result column names for arithmetic functions #33500

SQL: Fix result column names for arithmetic functions #33500

Conversation

matriv commented Sep 7, 2018

costin left a comment

Choose a reason for hiding this comment

costin Sep 7, 2018

Choose a reason for hiding this comment

matriv Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

matriv Sep 7, 2018

Choose a reason for hiding this comment

elasticmachine commented Sep 7, 2018

astefan left a comment

Choose a reason for hiding this comment

nik9000 Sep 7, 2018

Choose a reason for hiding this comment

costin Sep 7, 2018

Choose a reason for hiding this comment

costin Sep 7, 2018

Choose a reason for hiding this comment

matriv commented Sep 7, 2018

nik9000 left a comment

Choose a reason for hiding this comment

costin Sep 10, 2018

Choose a reason for hiding this comment

matriv Sep 10, 2018

Choose a reason for hiding this comment

matriv commented Sep 10, 2018

matriv Sep 7, 2018 •

edited

Loading