From 4dee4bf3b34081062ba2e28ab8524a066812a180 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 16 Oct 2019 12:45:57 +0200 Subject: [PATCH] SQL: Fix issue with negative literels and parentheses (#48113) 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 ++++++++++-- .../elasticsearch/xpack/sql/TestUtils.java | 11 +++++- .../sql/parser/EscapedFunctionsTests.java | 10 +---- .../xpack/sql/parser/ExpressionTests.java | 38 +++++++++++++++++-- 4 files changed, 66 insertions(+), 18 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/TestUtils.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/TestUtils.java index edce320c5b484..d34de666248a5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/TestUtils.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/TestUtils.java @@ -13,16 +13,18 @@ import org.elasticsearch.xpack.sql.util.DateUtils; import java.time.ZoneId; +import java.util.StringJoiner; import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength; import static org.elasticsearch.test.ESTestCase.randomBoolean; import static org.elasticsearch.test.ESTestCase.randomFrom; +import static org.elasticsearch.test.ESTestCase.randomInt; import static org.elasticsearch.test.ESTestCase.randomIntBetween; import static org.elasticsearch.test.ESTestCase.randomNonNegativeLong; import static org.elasticsearch.test.ESTestCase.randomZone; -public class TestUtils { +public final class TestUtils { private TestUtils() {} @@ -58,4 +60,11 @@ public static Configuration randomConfiguration(ZoneId providedZoneId) { randomBoolean()); } + public static String randomWhitespaces() { + StringJoiner sj = new StringJoiner(""); + for (int i = 0; i < randomInt(10); i++) { + sj.add(randomFrom(" ", "\t", "\r", "\n")); + } + return sj.toString(); + } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java index d4f1946653448..baf4de8641af0 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/EscapedFunctionsTests.java @@ -22,10 +22,10 @@ import java.util.List; import java.util.Locale; -import java.util.StringJoiner; import static java.lang.String.format; import static java.util.Arrays.asList; +import static org.elasticsearch.xpack.sql.TestUtils.randomWhitespaces; import static org.hamcrest.Matchers.endsWith; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -35,14 +35,6 @@ public class EscapedFunctionsTests extends ESTestCase { private final SqlParser parser = new SqlParser(); - private String randomWhitespaces() { - StringJoiner sj = new StringJoiner(""); - for (int i = 0; i < randomInt(10); i++) { - sj.add(randomFrom(" ", "\t", "\r", "\n")); - } - return sj.toString(); - } - private String buildExpression(String escape, String pattern, Object value) { return format(Locale.ROOT, "{" + randomWhitespaces() + escape + " " + randomWhitespaces() + pattern + randomWhitespaces() + "}", value); 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..c9fb153f57e02 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 @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.parser; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.TestUtils; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.function.UnresolvedFunction; @@ -202,10 +203,39 @@ public void testNegativeLiteral() { Expression expr = parser.createExpression("- 6"); assertEquals(Literal.class, expr.getClass()); assertEquals("- 6", expr.sourceText()); + assertEquals(-6, expr.fold()); + + expr = parser.createExpression("- ( 6.134 )"); + assertEquals(Literal.class, expr.getClass()); + assertEquals("- ( 6.134 )", expr.sourceText()); + assertEquals(-6.134, expr.fold()); + + expr = parser.createExpression("- ( ( (1.25) ) )"); + assertEquals(Literal.class, expr.getClass()); + assertEquals("- ( ( (1.25) ) )", expr.sourceText()); + assertEquals(-1.25, expr.fold()); + + int numberOfParentheses = randomIntBetween(3, 10); + double value = randomDouble(); + StringBuilder sb = new StringBuilder("-"); + for (int i = 0; i < numberOfParentheses; i++) { + sb.append("(").append(TestUtils.randomWhitespaces()); + } + sb.append(value); + for (int i = 0; i < numberOfParentheses; i++) { + sb.append(")"); + if (i < numberOfParentheses - 1) { + sb.append(TestUtils.randomWhitespaces()); + } + } + expr = parser.createExpression(sb.toString()); + assertEquals(Literal.class, expr.getClass()); + assertEquals(sb.toString(), expr.sourceText()); + assertEquals(- value, expr.fold()); } 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 +243,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());