Parser warnings when the query contains mixed AND/OR operator without…

… proper parenthesis.
prestodb · Sep 16, 2020 · 5bdb41d · 5bdb41d
1 parent df8228f
commit 5bdb41d
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 3 deletions.
diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java b/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java
@@ -184,6 +184,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
 import static com.facebook.presto.sql.tree.RoutineCharacteristics.Determinism;
@@ -203,10 +204,12 @@ class AstBuilder
 {
     private int parameterPosition;
     private final ParsingOptions parsingOptions;
+    private final Consumer<ParsingWarning> warningConsumer;
 
     AstBuilder(ParsingOptions parsingOptions)
     {
         this.parsingOptions = requireNonNull(parsingOptions, "parsingOptions is null");
+        this.warningConsumer = requireNonNull(parsingOptions.getWarningConsumer(), "warningConsumer is null");
     }
 
     @Override
@@ -1071,11 +1074,36 @@ public Node visitLogicalNot(SqlBaseParser.LogicalNotContext context)
     @Override
     public Node visitLogicalBinary(SqlBaseParser.LogicalBinaryContext context)
     {
+        LogicalBinaryExpression.Operator operator = getLogicalBinaryOperator(context.operator);
+        boolean warningForMixedAndOr = false;
+        Expression left = (Expression) visit(context.left);
+        Expression right = (Expression) visit(context.right);
+
+        if (operator.equals(LogicalBinaryExpression.Operator.OR) &&
+                (mixedAndOrOperatorParenthesisCheck(right, context.right, LogicalBinaryExpression.Operator.AND) ||
+                 mixedAndOrOperatorParenthesisCheck(left, context.left, LogicalBinaryExpression.Operator.AND))) {
+            warningForMixedAndOr = true;
+        }
+
+        if (operator.equals(LogicalBinaryExpression.Operator.AND) &&
+                (mixedAndOrOperatorParenthesisCheck(right, context.right, LogicalBinaryExpression.Operator.OR) ||
+                 mixedAndOrOperatorParenthesisCheck(left, context.left, LogicalBinaryExpression.Operator.OR))) {
+            warningForMixedAndOr = true;
+        }
+
+        if (warningForMixedAndOr) {
+            warningConsumer.accept(new ParsingWarning(
+                    "The Query contains OR and AND operator without proper parenthesis. "
+                    + "Make sure the operators are guarded by parenthesis in order "
+                    + "to fetch logically correct results",
+                    context.getStart().getLine(), context.getStart().getCharPositionInLine()));
+        }
+
         return new LogicalBinaryExpression(
                 getLocation(context.operator),
-                getLogicalBinaryOperator(context.operator),
-                (Expression) visit(context.left),
-                (Expression) visit(context.right));
+                operator,
+                left,
+                right);
     }
 
     // *************** from clause *****************
@@ -2410,6 +2438,22 @@ else if (context instanceof SqlBaseParser.RolePrincipalContext) {
         }
     }
 
+    private boolean mixedAndOrOperatorParenthesisCheck(Expression expression, SqlBaseParser.BooleanExpressionContext node, LogicalBinaryExpression.Operator operator)
+    {
+        if (expression instanceof LogicalBinaryExpression) {
+            if (((LogicalBinaryExpression) expression).getOperator().equals(operator)) {
+                if (node.children.get(0) instanceof SqlBaseParser.ValueExpressionDefaultContext) {
+                    return !(((SqlBaseParser.PredicatedContext) node).valueExpression().getChild(0) instanceof
+                        SqlBaseParser.ParenthesizedExpressionContext);
+                }
+                else {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     private static void check(boolean condition, String message, ParserRuleContext context)
     {
         if (!condition) {

diff --git a/presto-tests/src/test/java/com/facebook/presto/execution/TestWarnings.java b/presto-tests/src/test/java/com/facebook/presto/execution/TestWarnings.java
@@ -101,6 +101,46 @@ public void testQuotedIdentifiersDoNotTriggerWarning()
         assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
     }
 
+    @Test
+    public void testMixAndOrWarnings()
+    {
+        String query = "select true or false";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select true or false and false";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+
+        query = "select true or (false and false)";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select true or false or false";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select true and false and false";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select (true or false) and false";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select true and false or false and true";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+
+        query = "select true or false and false or true";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+
+        query = "select (true or false) and false or true";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+
+        query = "select true or false and (false or true)";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+
+        query = "select (true or false) and (false or true)";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of());
+
+        query = "select (true and true) or (true and false or false and true)";
+        assertWarnings(queryRunner, TEST_SESSION, query, ImmutableSet.of(PARSER_WARNING.toWarningCode()));
+    }
+
     private static void assertWarnings(QueryRunner queryRunner, Session session, @Language("SQL") String sql, Set<WarningCode> expectedWarnings)
     {
         Set<WarningCode> warnings = queryRunner.execute(session, sql).getWarnings().stream()