Skip to content

Commit 74fe2f4

Browse files
matrivmatriv
committed
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 (cherry picked from commit 4dee4bf)
1 parent bca0c8d commit 74fe2f4

File tree

4 files changed

+68
-20
lines changed

4 files changed

+68
-20
lines changed

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ public Literal visitInterval(IntervalContext interval) {
509509

510510
TimeUnit leading = visitIntervalField(interval.leading);
511511
TimeUnit trailing = visitIntervalField(interval.trailing);
512-
512+
513513
// only YEAR TO MONTH or DAY TO HOUR/MINUTE/SECOND are valid declaration
514514
if (trailing != null) {
515515
if (leading == TimeUnit.YEAR && trailing != TimeUnit.MONTH) {
@@ -862,11 +862,28 @@ private static Source minusAwareSource(SqlBaseParser.NumberContext ctx) {
862862
if (parentCtx != null) {
863863
if (parentCtx instanceof SqlBaseParser.NumericLiteralContext) {
864864
parentCtx = parentCtx.getParent();
865-
if (parentCtx != null && parentCtx instanceof SqlBaseParser.ConstantDefaultContext) {
865+
if (parentCtx instanceof ConstantDefaultContext) {
866866
parentCtx = parentCtx.getParent();
867-
if (parentCtx != null && parentCtx instanceof SqlBaseParser.ValueExpressionDefaultContext) {
867+
if (parentCtx instanceof ValueExpressionDefaultContext) {
868868
parentCtx = parentCtx.getParent();
869-
if (parentCtx != null && parentCtx instanceof SqlBaseParser.ArithmeticUnaryContext) {
869+
870+
// Skip parentheses, e.g.: - (( (2.15) ) )
871+
while (parentCtx instanceof PredicatedContext) {
872+
parentCtx = parentCtx.getParent();
873+
if (parentCtx instanceof SqlBaseParser.BooleanDefaultContext) {
874+
parentCtx = parentCtx.getParent();
875+
}
876+
if (parentCtx instanceof SqlBaseParser.ExpressionContext) {
877+
parentCtx = parentCtx.getParent();
878+
}
879+
if (parentCtx instanceof SqlBaseParser.ParenthesizedExpressionContext) {
880+
parentCtx = parentCtx.getParent();
881+
}
882+
if (parentCtx instanceof ValueExpressionDefaultContext) {
883+
parentCtx = parentCtx.getParent();
884+
}
885+
}
886+
if (parentCtx instanceof ArithmeticUnaryContext) {
870887
if (((ArithmeticUnaryContext) parentCtx).MINUS() != null) {
871888
return source(parentCtx);
872889
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/TestUtils.java‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,17 @@
1616
import java.time.Duration;
1717
import java.time.ZonedDateTime;
1818
import java.time.ZoneId;
19+
import java.util.StringJoiner;
1920

2021
import static org.elasticsearch.test.ESTestCase.randomAlphaOfLength;
2122
import static org.elasticsearch.test.ESTestCase.randomFrom;
23+
import static org.elasticsearch.test.ESTestCase.randomInt;
2224
import static org.elasticsearch.test.ESTestCase.randomIntBetween;
2325
import static org.elasticsearch.test.ESTestCase.randomNonNegativeLong;
2426
import static org.elasticsearch.test.ESTestCase.randomZone;
2527

2628

27-
public class TestUtils {
29+
public final class TestUtils {
2830

2931
private TestUtils() {}
3032

@@ -38,10 +40,10 @@ private TestUtils() {}
3840
* that the precision of the clock can be milliseconds, microseconds or nanoseconds), whereas in Java 8
3941
* {@code System.currentTimeMillis()} is always used. To account for these differences, this method defines a new {@code Clock}
4042
* which will offer a value for {@code ZonedDateTime.now()} set to always have milliseconds precision.
41-
*
43+
*
4244
* @return {@link ZonedDateTime} instance for the current date-time with milliseconds precision in UTC
4345
*/
44-
public static final ZonedDateTime now() {
46+
public static ZonedDateTime now() {
4547
return ZonedDateTime.now(Clock.tick(Clock.system(DateUtils.UTC), Duration.ofMillis(1)));
4648
}
4749

@@ -71,5 +73,12 @@ public static Configuration randomConfiguration(ZoneId providedZoneId) {
7173
false);
7274
}
7375

76+
public static String randomWhitespaces() {
77+
StringJoiner sj = new StringJoiner("");
78+
for (int i = 0; i < randomInt(10); i++) {
79+
sj.add(randomFrom(" ", "\t", "\r", "\n"));
80+
}
81+
return sj.toString();
82+
}
7483
}
7584

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

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@
2323

2424
import java.util.List;
2525
import java.util.Locale;
26-
import java.util.StringJoiner;
2726

2827
import static java.lang.String.format;
2928
import static java.util.Arrays.asList;
3029
import static org.elasticsearch.test.TestMatchers.matchesPattern;
30+
import static org.elasticsearch.xpack.sql.TestUtils.randomWhitespaces;
3131
import static org.hamcrest.Matchers.endsWith;
3232
import static org.hamcrest.Matchers.instanceOf;
3333
import static org.hamcrest.Matchers.is;
@@ -36,14 +36,6 @@ public class EscapedFunctionsTests extends ESTestCase {
3636

3737
private final SqlParser parser = new SqlParser();
3838

39-
private String randomWhitespaces() {
40-
StringJoiner sj = new StringJoiner("");
41-
for (int i = 0; i < randomInt(10); i++) {
42-
sj.add(randomFrom(" ", "\t", "\r", "\n"));
43-
}
44-
return sj.toString();
45-
}
46-
4739
private String buildExpression(String escape, String pattern, Object value) {
4840
return format(Locale.ROOT, "{" + randomWhitespaces() + escape + " " + randomWhitespaces() +
4941
pattern + randomWhitespaces() + "}", value);

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

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.elasticsearch.xpack.sql.parser;
77

88
import org.elasticsearch.test.ESTestCase;
9+
import org.elasticsearch.xpack.sql.TestUtils;
910
import org.elasticsearch.xpack.sql.expression.Expression;
1011
import org.elasticsearch.xpack.sql.expression.Literal;
1112
import org.elasticsearch.xpack.sql.expression.function.UnresolvedFunction;
@@ -199,26 +200,55 @@ public void testNegativeLiteral() {
199200
Expression expr = parser.createExpression("- 6");
200201
assertEquals(Literal.class, expr.getClass());
201202
assertEquals("- 6", expr.sourceText());
203+
assertEquals(-6, expr.fold());
204+
205+
expr = parser.createExpression("- ( 6.134 )");
206+
assertEquals(Literal.class, expr.getClass());
207+
assertEquals("- ( 6.134 )", expr.sourceText());
208+
assertEquals(-6.134, expr.fold());
209+
210+
expr = parser.createExpression("- ( ( (1.25) ) )");
211+
assertEquals(Literal.class, expr.getClass());
212+
assertEquals("- ( ( (1.25) ) )", expr.sourceText());
213+
assertEquals(-1.25, expr.fold());
214+
215+
int numberOfParentheses = randomIntBetween(3, 10);
216+
double value = randomDouble();
217+
StringBuilder sb = new StringBuilder("-");
218+
for (int i = 0; i < numberOfParentheses; i++) {
219+
sb.append("(").append(TestUtils.randomWhitespaces());
220+
}
221+
sb.append(value);
222+
for (int i = 0; i < numberOfParentheses; i++) {
223+
sb.append(")");
224+
if (i < numberOfParentheses - 1) {
225+
sb.append(TestUtils.randomWhitespaces());
226+
}
227+
}
228+
expr = parser.createExpression(sb.toString());
229+
assertEquals(Literal.class, expr.getClass());
230+
assertEquals(sb.toString(), expr.sourceText());
231+
assertEquals(- value, expr.fold());
202232
}
203233

204234
public void testComplexArithmetic() {
205-
String sql = "-(((a-2)-(-3))+b)";
235+
String sql = "-(((a-2)- (( - ( ( 3) )) ) )+ b)";
206236
Expression expr = parser.createExpression(sql);
207237
assertEquals(Neg.class, expr.getClass());
208238
Neg neg = (Neg) expr;
209239
assertEquals(sql, neg.sourceText());
210240
assertEquals(1, neg.children().size());
211241
assertEquals(Add.class, neg.children().get(0).getClass());
212242
Add add = (Add) neg.children().get(0);
213-
assertEquals("((a-2)-(-3))+b", add.sourceText());
243+
assertEquals("((a-2)- (( - ( ( 3) )) ) )+ b", add.sourceText());
214244
assertEquals(2, add.children().size());
215245
assertEquals("?b", add.children().get(1).toString());
216246
assertEquals(Sub.class, add.children().get(0).getClass());
217247
Sub sub1 = (Sub) add.children().get(0);
218-
assertEquals("(a-2)-(-3)", sub1.sourceText());
248+
assertEquals("(a-2)- (( - ( ( 3) )) )", sub1.sourceText());
219249
assertEquals(2, sub1.children().size());
220250
assertEquals(Literal.class, sub1.children().get(1).getClass());
221-
assertEquals("-3", ((Literal) sub1.children().get(1)).sourceText());
251+
assertEquals("- ( ( 3) )", sub1.children().get(1).sourceText());
222252
assertEquals(Sub.class, sub1.children().get(0).getClass());
223253
Sub sub2 = (Sub) sub1.children().get(0);
224254
assertEquals(2, sub2.children().size());

0 commit comments

Comments
 (0)