Skip to content

Commit ab46fdd

Browse files
dai-chendai-chen
authored
Fix arithmetic operator precedence (#1172)
* Fix precedence by reordering grammar rule Signed-off-by: Chen Dai <daichen@amazon.com> * Fix precedence in PPL Signed-off-by: Chen Dai <daichen@amazon.com> Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent 5073215 commit ab46fdd

6 files changed

Lines changed: 53 additions & 21 deletions

File tree

‎ppl/src/main/antlr/OpenSearchPPLParser.g4‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -254,11 +254,15 @@ comparisonExpression
254254
;
255255

256256
valueExpression
257-
: left=valueExpression binaryOperator right=valueExpression #binaryArithmetic
258-
| LT_PRTHS left=valueExpression binaryOperator
259-
right=valueExpression RT_PRTHS #parentheticBinaryArithmetic
257+
: left=valueExpression
258+
binaryOperator=(STAR | DIVIDE | MODULE)
259+
right=valueExpression #binaryArithmetic
260+
| left=valueExpression
261+
binaryOperator=(PLUS | MINUS)
262+
right=valueExpression #binaryArithmetic
260263
| primaryExpression #valueExpressionDefault
261264
| positionFunction #positionFunctionCall
265+
| LT_PRTHS valueExpression RT_PRTHS #parentheticValueExpr
262266
;
263267

264268
primaryExpression
@@ -499,10 +503,6 @@ comparisonOperator
499503
: EQUAL | NOT_EQUAL | LESS | NOT_LESS | GREATER | NOT_GREATER | REGEXP
500504
;
501505

502-
binaryOperator
503-
: PLUS | MINUS | STAR | DIVIDE | MODULE
504-
;
505-
506506

507507
singleFieldRelevanceFunctionName
508508
: MATCH

‎ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.LogicalOrContext;
3434
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.LogicalXorContext;
3535
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.MultiFieldRelevanceFunctionContext;
36-
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.ParentheticBinaryArithmeticContext;
36+
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.ParentheticValueExprContext;
3737
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PercentileAggFunctionContext;
3838
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SingleFieldRelevanceFunctionContext;
3939
import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext;
@@ -154,18 +154,14 @@ public UnresolvedExpression visitInExpr(InExprContext ctx) {
154154
@Override
155155
public UnresolvedExpression visitBinaryArithmetic(BinaryArithmeticContext ctx) {
156156
return new Function(
157-
ctx.binaryOperator().getText(),
157+
ctx.binaryOperator.getText(),
158158
Arrays.asList(visit(ctx.left), visit(ctx.right))
159159
);
160160
}
161161

162162
@Override
163-
public UnresolvedExpression visitParentheticBinaryArithmetic(
164-
ParentheticBinaryArithmeticContext ctx) {
165-
return new Function(
166-
ctx.binaryOperator().getText(),
167-
Arrays.asList(visit(ctx.left), visit(ctx.right))
168-
);
163+
public UnresolvedExpression visitParentheticValueExpr(ParentheticValueExprContext ctx) {
164+
return visit(ctx.valueExpression()); // Discard parenthesis around
169165
}
170166

171167
/**

‎ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,30 @@ public void testLiteralValueBinaryOperationExpr() {
226226
));
227227
}
228228

229+
@Test
230+
public void testBinaryOperationExprWithParentheses() {
231+
assertEqual("source = t | where a = (1 + 2) * 3",
232+
filter(
233+
relation("t"),
234+
compare("=",
235+
field("a"),
236+
function("*",
237+
function("+", intLiteral(1), intLiteral(2)),
238+
intLiteral(3)))));
239+
}
240+
241+
@Test
242+
public void testBinaryOperationExprPrecedence() {
243+
assertEqual("source = t | where a = 1 + 2 * 3",
244+
filter(
245+
relation("t"),
246+
compare("=",
247+
field("a"),
248+
function("+",
249+
intLiteral(1),
250+
function("*", intLiteral(2), intLiteral(3))))));
251+
}
252+
229253
@Test
230254
public void testCompareExpr() {
231255
assertEqual("source=t a='b'",

‎sql/src/main/antlr/OpenSearchSQLParser.g4‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -292,11 +292,12 @@ expressionAtom
292292
| columnName #fullColumnNameExpressionAtom
293293
| functionCall #functionCallExpressionAtom
294294
| LR_BRACKET expression RR_BRACKET #nestedExpressionAtom
295-
| left=expressionAtom mathOperator right=expressionAtom #mathExpressionAtom
296-
;
297-
298-
mathOperator
299-
: PLUS | MINUS | STAR | DIVIDE | MODULE
295+
| left=expressionAtom
296+
mathOperator=(STAR | DIVIDE | MODULE)
297+
right=expressionAtom #mathExpressionAtom
298+
| left=expressionAtom
299+
mathOperator=(PLUS | MINUS)
300+
right=expressionAtom #mathExpressionAtom
300301
;
301302

302303
comparisonOperator

‎sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public UnresolvedExpression visitQualifiedName(QualifiedNameContext ctx) {
120120
@Override
121121
public UnresolvedExpression visitMathExpressionAtom(MathExpressionAtomContext ctx) {
122122
return new Function(
123-
ctx.mathOperator().getText(),
123+
ctx.mathOperator.getText(),
124124
Arrays.asList(visit(ctx.left), visit(ctx.right))
125125
);
126126
}

‎sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,17 @@ public void canBuildArithmeticExpression() {
153153
);
154154
}
155155

156+
@Test
157+
public void canBuildArithmeticExpressionPrecedence() {
158+
assertEquals(
159+
function("+",
160+
intLiteral(1),
161+
function("*",
162+
intLiteral(2), intLiteral(3))),
163+
buildExprAst("1 + 2 * 3")
164+
);
165+
}
166+
156167
@Test
157168
public void canBuildFunctionWithoutArguments() {
158169
assertEquals(

0 commit comments

Comments
 (0)