From d0a1c2bb75253abfc0e03e68d46019ea72c2a366 Mon Sep 17 00:00:00 2001
From: Marios Trivyzas <matriv@users.noreply.github.com>
Date: Wed, 16 Oct 2019 11:09:02 +0200
Subject: [PATCH] SQL: Fix issue with negative literels and parentheses

Previously when a numeric literal was enclosed in parentheses and then
negated, the negation was lost and the number was considered positive, e.g.:
`-(5)` was considered as `5` instead of `-5`
`- ( (1.28) )` was considered as `1.28` instead of `-1.28`

Fixes: #48009
---
 .../xpack/sql/parser/ExpressionBuilder.java   | 25 ++++++++++++++++---
 .../xpack/sql/parser/ExpressionTests.java     | 16 +++++++++---
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
index 88b4cefd9b982..5a1e09f602a48 100644
--- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
+++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java
@@ -528,7 +528,7 @@ public Literal visitInterval(IntervalContext interval) {
 
         TimeUnit leading = visitIntervalField(interval.leading);
         TimeUnit trailing = visitIntervalField(interval.trailing);
-        
+
         // only YEAR TO MONTH or DAY TO HOUR/MINUTE/SECOND are valid declaration
         if (trailing != null) {
             if (leading == TimeUnit.YEAR && trailing != TimeUnit.MONTH) {
@@ -869,11 +869,28 @@ private static Source minusAwareSource(SqlBaseParser.NumberContext ctx) {
         if (parentCtx != null) {
             if (parentCtx instanceof SqlBaseParser.NumericLiteralContext) {
                 parentCtx = parentCtx.getParent();
-                if (parentCtx != null && parentCtx instanceof SqlBaseParser.ConstantDefaultContext) {
+                if (parentCtx instanceof ConstantDefaultContext) {
                     parentCtx = parentCtx.getParent();
-                    if (parentCtx != null && parentCtx instanceof SqlBaseParser.ValueExpressionDefaultContext) {
+                    if (parentCtx instanceof ValueExpressionDefaultContext) {
                         parentCtx = parentCtx.getParent();
-                        if (parentCtx != null && parentCtx instanceof SqlBaseParser.ArithmeticUnaryContext) {
+
+                        // Skip parentheses, e.g.: - (( (2.15) ) )
+                        while (parentCtx instanceof PredicatedContext) {
+                            parentCtx = parentCtx.getParent();
+                            if (parentCtx instanceof SqlBaseParser.BooleanDefaultContext) {
+                                parentCtx = parentCtx.getParent();
+                            }
+                            if (parentCtx instanceof SqlBaseParser.ExpressionContext) {
+                                parentCtx = parentCtx.getParent();
+                            }
+                            if (parentCtx instanceof SqlBaseParser.ParenthesizedExpressionContext) {
+                                parentCtx = parentCtx.getParent();
+                            }
+                            if (parentCtx instanceof ValueExpressionDefaultContext) {
+                                parentCtx = parentCtx.getParent();
+                            }
+                        }
+                        if (parentCtx instanceof ArithmeticUnaryContext) {
                             if (((ArithmeticUnaryContext) parentCtx).MINUS() != null) {
                                 return source(parentCtx);
                             }
diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
index aff8d7b8fd774..79e19335b13e8 100644
--- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
+++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java
@@ -202,10 +202,18 @@ public void testNegativeLiteral() {
         Expression expr = parser.createExpression("- 6");
         assertEquals(Literal.class, expr.getClass());
         assertEquals("- 6", expr.sourceText());
+
+        expr = parser.createExpression("- ( 6 )");
+        assertEquals(Literal.class, expr.getClass());
+        assertEquals("- ( 6 )", expr.sourceText());
+
+        expr = parser.createExpression("- ( ( (1.25) )    )");
+        assertEquals(Literal.class, expr.getClass());
+        assertEquals("- ( ( (1.25) )    )", expr.sourceText());
     }
 
     public void testComplexArithmetic() {
-        String sql = "-(((a-2)-(-3))+b)";
+        String sql = "-(((a-2)- (( - ( (  3)  )) )  )+ b)";
         Expression expr = parser.createExpression(sql);
         assertEquals(Neg.class, expr.getClass());
         Neg neg = (Neg) expr;
@@ -213,15 +221,15 @@ public void testComplexArithmetic() {
         assertEquals(1, neg.children().size());
         assertEquals(Add.class, neg.children().get(0).getClass());
         Add add = (Add) neg.children().get(0);
-        assertEquals("((a-2)-(-3))+b", add.sourceText());
+        assertEquals("((a-2)- (( - ( (  3)  )) )  )+ b", add.sourceText());
         assertEquals(2, add.children().size());
         assertEquals("?b", add.children().get(1).toString());
         assertEquals(Sub.class, add.children().get(0).getClass());
         Sub sub1 = (Sub) add.children().get(0);
-        assertEquals("(a-2)-(-3)", sub1.sourceText());
+        assertEquals("(a-2)- (( - ( (  3)  )) )", sub1.sourceText());
         assertEquals(2, sub1.children().size());
         assertEquals(Literal.class, sub1.children().get(1).getClass());
-        assertEquals("-3", sub1.children().get(1).sourceText());
+        assertEquals("- ( (  3)  )", sub1.children().get(1).sourceText());
         assertEquals(Sub.class, sub1.children().get(0).getClass());
         Sub sub2 = (Sub) sub1.children().get(0);
         assertEquals(2, sub2.children().size());