Fix arithmetic operator precedence (opensearch-project#1172)

* Fix precedence by reordering grammar rule Signed-off-by: Chen Dai <[email protected]> * Fix precedence in PPL Signed-off-by: Chen Dai <[email protected]> Signed-off-by: Chen Dai <[email protected]>
Bit-Quill · Dec 16, 2022 · ab46fdd · ab46fdd
1 parent 5073215
commit ab46fdd
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 21 deletions.
diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4
@@ -254,11 +254,15 @@ comparisonExpression
     ;
 
 valueExpression
-    : left=valueExpression binaryOperator right=valueExpression     #binaryArithmetic
-    | LT_PRTHS left=valueExpression binaryOperator
-    right=valueExpression RT_PRTHS                                  #parentheticBinaryArithmetic
+    : left=valueExpression
+        binaryOperator=(STAR | DIVIDE | MODULE)
+            right=valueExpression                                   #binaryArithmetic
+    | left=valueExpression
+        binaryOperator=(PLUS | MINUS)
+            right=valueExpression                                   #binaryArithmetic
     | primaryExpression                                             #valueExpressionDefault
     | positionFunction                                              #positionFunctionCall
+    | LT_PRTHS valueExpression RT_PRTHS                             #parentheticValueExpr
     ;
 
 primaryExpression
@@ -499,10 +503,6 @@ comparisonOperator
     : EQUAL | NOT_EQUAL | LESS | NOT_LESS | GREATER | NOT_GREATER | REGEXP
     ;
 
-binaryOperator
-    : PLUS | MINUS | STAR | DIVIDE | MODULE
-    ;
-
 
 singleFieldRelevanceFunctionName
     : MATCH

diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
@@ -33,7 +33,7 @@
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.LogicalOrContext;
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.LogicalXorContext;
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.MultiFieldRelevanceFunctionContext;
-import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.ParentheticBinaryArithmeticContext;
+import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.ParentheticValueExprContext;
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.PercentileAggFunctionContext;
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SingleFieldRelevanceFunctionContext;
 import static org.opensearch.sql.ppl.antlr.parser.OpenSearchPPLParser.SortFieldContext;
@@ -154,18 +154,14 @@ public UnresolvedExpression visitInExpr(InExprContext ctx) {
   @Override
   public UnresolvedExpression visitBinaryArithmetic(BinaryArithmeticContext ctx) {
     return new Function(
-        ctx.binaryOperator().getText(),
+        ctx.binaryOperator.getText(),
         Arrays.asList(visit(ctx.left), visit(ctx.right))
     );
   }
 
   @Override
-  public UnresolvedExpression visitParentheticBinaryArithmetic(
-      ParentheticBinaryArithmeticContext ctx) {
-    return new Function(
-        ctx.binaryOperator().getText(),
-        Arrays.asList(visit(ctx.left), visit(ctx.right))
-    );
+  public UnresolvedExpression visitParentheticValueExpr(ParentheticValueExprContext ctx) {
+    return visit(ctx.valueExpression()); // Discard parenthesis around
   }
 
   /**

diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java
@@ -226,6 +226,30 @@ public void testLiteralValueBinaryOperationExpr() {
         ));
   }
 
+  @Test
+  public void testBinaryOperationExprWithParentheses() {
+    assertEqual("source = t | where a = (1 + 2) * 3",
+        filter(
+            relation("t"),
+            compare("=",
+                field("a"),
+                function("*",
+                    function("+", intLiteral(1), intLiteral(2)),
+                    intLiteral(3)))));
+  }
+
+  @Test
+  public void testBinaryOperationExprPrecedence() {
+    assertEqual("source = t | where a = 1 + 2 * 3",
+        filter(
+            relation("t"),
+            compare("=",
+                field("a"),
+                function("+",
+                    intLiteral(1),
+                    function("*", intLiteral(2), intLiteral(3))))));
+  }
+
   @Test
   public void testCompareExpr() {
     assertEqual("source=t a='b'",

diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4
@@ -292,11 +292,12 @@ expressionAtom
     | columnName                                                    #fullColumnNameExpressionAtom
     | functionCall                                                  #functionCallExpressionAtom
     | LR_BRACKET expression RR_BRACKET                              #nestedExpressionAtom
-    | left=expressionAtom mathOperator right=expressionAtom         #mathExpressionAtom
-    ;
-
-mathOperator
-    : PLUS | MINUS | STAR | DIVIDE | MODULE
+    | left=expressionAtom
+        mathOperator=(STAR | DIVIDE | MODULE)
+            right=expressionAtom                                    #mathExpressionAtom
+    | left=expressionAtom
+        mathOperator=(PLUS | MINUS)
+            right=expressionAtom                                    #mathExpressionAtom
     ;
 
 comparisonOperator

diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java
@@ -120,7 +120,7 @@ public UnresolvedExpression visitQualifiedName(QualifiedNameContext ctx) {
   @Override
   public UnresolvedExpression visitMathExpressionAtom(MathExpressionAtomContext ctx) {
     return new Function(
-        ctx.mathOperator().getText(),
+        ctx.mathOperator.getText(),
         Arrays.asList(visit(ctx.left), visit(ctx.right))
     );
   }

diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java
@@ -153,6 +153,17 @@ public void canBuildArithmeticExpression() {
     );
   }
 
+  @Test
+  public void canBuildArithmeticExpressionPrecedence() {
+    assertEquals(
+        function("+",
+            intLiteral(1),
+            function("*",
+                intLiteral(2), intLiteral(3))),
+        buildExprAst("1 + 2 * 3")
+    );
+  }
+
   @Test
   public void canBuildFunctionWithoutArguments() {
     assertEquals(