From ab46fdd97b4a766954820edb49799d2972eece06 Mon Sep 17 00:00:00 2001 From: Chen Dai Date: Fri, 16 Dec 2022 09:37:50 -0800 Subject: [PATCH] Fix arithmetic operator precedence (#1172) * Fix precedence by reordering grammar rule Signed-off-by: Chen Dai * Fix precedence in PPL Signed-off-by: Chen Dai Signed-off-by: Chen Dai --- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 14 +++++------ .../sql/ppl/parser/AstExpressionBuilder.java | 12 ++++------ .../ppl/parser/AstExpressionBuilderTest.java | 24 +++++++++++++++++++ sql/src/main/antlr/OpenSearchSQLParser.g4 | 11 +++++---- .../sql/sql/parser/AstExpressionBuilder.java | 2 +- .../sql/parser/AstExpressionBuilderTest.java | 11 +++++++++ 6 files changed, 53 insertions(+), 21 deletions(-) diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index a0d6553875..d7491abb51 100644 --- 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 index 68608e23ad..d4df4cf7dd 100644 --- 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 index dbdfb71aa7..cd0787695a 100644 --- 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 index 58d4be1813..bc76480d18 100644 --- 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 index bae22595ca..4b1a15425c 100644 --- 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 index 9af4119fdf..31eb3d2f05 100644 --- 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(