SQL: Fix handling of nested minuses (#70555)

Previously, the parentCtx checks on a unary arithmetic expression
stopped too early, resulting in wrong handling of nested minuses.

Use a while loop to reach the top level unary arithmetic expression
and count the number of minuses, an odd count means an acutal negation.

Fixes: #66145

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

@@ -95,6 +95,7 @@ import org.elasticsearch.xpack.sql.parser.SqlBaseParser.LogicalNotContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.MatchQueryContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.MatchQueryOptionsContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.MultiMatchQueryContext;
+import org.elasticsearch.xpack.sql.parser.SqlBaseParser.NamedValueExpressionContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.NullLiteralContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.NumberContext;
 import org.elasticsearch.xpack.sql.parser.SqlBaseParser.OrderByContext;
@@ -856,24 +857,19 @@ abstract class ExpressionBuilder extends IdentifierBuilder {
      */
     private static Tuple<Source, String> withMinus(NumberContext ctx) {
         String string = ctx.getText();
-        Source source = minusAwareSource(ctx);
-
-        if (source != null) {
-            string = "-" + string;
-        } else {
-            source = source(ctx);
-        }
-
-        return new Tuple<>(source, string);
+        Tuple<Source, Boolean> tuple = minusAwareSource(ctx);
+        return new Tuple<>(tuple.v1(), (tuple.v2() ? "-" : "") + string);
     }
 
     /**
-     * Checks the presence of MINUS (-) in the parent and if found,
-     * returns the parent source or null otherwise.
+     * Checks the presence of MINUS (-) in the parent and if found, returns the parent source
+     * along with a boolean denoting if there is an odd number of MINUSes.
+     * If not found, returns the original source.
+     *
      * Parsing of the value should not depend on the returned source
      * as it might contain extra spaces.
      */
-    private static Source minusAwareSource(SqlBaseParser.NumberContext ctx) {
+    private static Tuple<Source, Boolean> minusAwareSource(SqlBaseParser.NumberContext ctx) {
         ParserRuleContext parentCtx = ctx.getParent();
         if (parentCtx != null) {
             if (parentCtx instanceof SqlBaseParser.NumericLiteralContext) {
@@ -883,40 +879,42 @@ abstract class ExpressionBuilder extends IdentifierBuilder {
                     if (parentCtx instanceof ValueExpressionDefaultContext) {
                         parentCtx = parentCtx.getParent();
 
-                        // Skip parentheses, e.g.: - (( (2.15) ) )
-                        while (parentCtx instanceof PredicatedContext) {
-                            parentCtx = parentCtx.getParent();
-                            if (parentCtx instanceof SqlBaseParser.BooleanDefaultContext) {
-                                parentCtx = parentCtx.getParent();
+                        ParserRuleContext returnCtx = null;
+                        boolean minus = false;
+                        while (parentCtx != null) {
+                            // Stop when we meet a higher level context to avoid unnecessary looping
+                            if (parentCtx instanceof ArithmeticBinaryContext
+                                || parentCtx instanceof ExtractTemplateContext
+                                || parentCtx instanceof NamedValueExpressionContext
+                                || parentCtx instanceof PredicateContext // Not PredicatedContext !!
+                                || parentCtx instanceof ComparisonContext) {
+                                break;
                             }
-                            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) {
+                                returnCtx = parentCtx;
+                                if (((ArithmeticUnaryContext) parentCtx).MINUS() != null) {
+                                    minus ^= true;
+                                }
                             }
+                            parentCtx = parentCtx.getParent();
                         }
-                        if (parentCtx instanceof ArithmeticUnaryContext) {
-                            if (((ArithmeticUnaryContext) parentCtx).MINUS() != null) {
-                                return source(parentCtx);
-                            }
+                        if (returnCtx != null) {
+                            return new Tuple<>(source(returnCtx), minus);
                         }
                     }
                 }
-            } else if (parentCtx instanceof SqlBaseParser.IntervalContext) {
+            // Intervals and SysTypes can only have a single "-" as parentheses are not allowed there
+            } else if (parentCtx instanceof IntervalContext) {
                 IntervalContext ic = (IntervalContext) parentCtx;
                 if (ic.sign != null && ic.sign.getType() == SqlBaseParser.MINUS) {
-                    return source(ic);
+                    return new Tuple<>(source(ic), true);
                 }
-            } else if (parentCtx instanceof SqlBaseParser.SysTypesContext) {
+            } else if (parentCtx instanceof SysTypesContext) {
                 if (((SysTypesContext) parentCtx).MINUS() != null) {
-                    return source(parentCtx);
+                    return new Tuple<>(source(parentCtx), true);
                 }
             }
         }
-        return null;
+        return new Tuple<>(source(ctx), false);
     }
 }

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/ExpressionTests.java

@@ -222,11 +222,26 @@ public class ExpressionTests extends ESTestCase {
         assertEquals("- ( ( (1.25) )    )", expr.sourceText());
         assertEquals(-1.25, expr.fold());
 
-        int numberOfParentheses = randomIntBetween(3, 10);
+        expr = parser.createExpression("- ( -( (-1.25) )    )");
+        assertEquals(Literal.class, expr.getClass());
+        assertEquals("- ( -( (-1.25) )    )", expr.sourceText());
+        assertEquals(-1.25, expr.fold());
+
+        expr = parser.createExpression("- ( -( -(-10) )    )");
+        assertEquals(Literal.class, expr.getClass());
+        assertEquals("- ( -( -(-10) )    )", expr.sourceText());
+        assertEquals(10, expr.fold());
+
+        int numberOfParentheses = randomIntBetween(3, 20);
+        int numberOfMinuses = 1;
         double value = randomDouble();
         StringBuilder sb = new StringBuilder("-");
         for (int i = 0; i < numberOfParentheses; i++) {
             sb.append("(").append(SqlTestUtils.randomWhitespaces());
+            if (randomBoolean()) {
+                sb.append("-");
+                numberOfMinuses++;
+            }
         }
         sb.append(value);
         for (int i = 0; i < numberOfParentheses; i++) {
@@ -238,7 +253,7 @@ public class ExpressionTests extends ESTestCase {
         expr = parser.createExpression(sb.toString());
         assertEquals(Literal.class, expr.getClass());
         assertEquals(sb.toString(), expr.sourceText());
-        assertEquals(- value, expr.fold());
+        assertEquals(numberOfMinuses % 2  == 0 ? value : - value, expr.fold());
     }
 
     public void testComplexArithmetic() {