Repository: kayak/pypika
Pull request #532 (2020-12-10 18:28:52)

Issue 508: Fix order of operations
#508

This change ensures that the following expressions are evaluated correctly:

```
Before change:
1 / (2 * 3) --> 1 / 2 * 3
1 - (2 - 3) --> 1 - 2 - 3

After change:
1 / (2 * 3) --> 1 / (2 * 3)
1 - (2 - 3) --> 1 - (2 - 3)
```

Quick note:

A simpler fix would be to add parentheses around the left/right expressions unconditionally (well, as long as the expressions are not single items) in the following way:

```
arithmetic_sql = "{left}{operator}{right}".format(
        operator=self.operator.value,
        # If left_op/right_op is None, then the left/right expression is a single item.
        left=("({})" if left_op is not None else "{}").format(self.left.get_sql(**kwargs)),
        right=("({})" if right_op is not None else "{}").format(self.right.get_sql(**kwargs)),
)
```

However, this would result in unnecessary parentheses (example below):

`
Query.select(1 + 2 + 3) --> SELECT (1 + 2) + 3
`

Which is technically correct but the intent of the original implementation was to avoid doing this, so we should preserve that behavior.

@alec-heif @awishnick

coveralls: 
[![Coverage Status](https://coveralls.io/builds/35611491/badge)](https://coveralls.io/builds/35611491)

Coverage increased (+0.03%) to 98.245% when pulling **b8eab0567020a4ac9e5fef260ee9e96cc0f432df on mkbsh:fix_arithmetic** into **1e2104eae8a282d7d7a9b56ed7a62f94eb14f830 on kayak:master**.

mkbsh: Some python 3.9 unit tests that are unrelated to this change are failing. Was the 3.9 check recently? I don't see any other pull requests running this check.

```
======================================================================
ERROR: test_delete_returning_str (pypika.tests.test_deletes.PostgresDeleteTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/pypika/pypika/pypika/tests/test_deletes.py", line 62, in test_delete_returning_str
    PostgreSQLQuery.from_(self.table_abc)
  File "/home/runner/work/pypika/pypika/pypika/utils.py", line 50, in _copy
    result = func(self_copy, *args, **kwargs)
  File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 476, in returning
    self._return_field_str(term)
  File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 518, in _return_field_str
    self._return_field(Field(term, table=self._from[0]))
  File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 500, in _return_field
    self._validate_returning_term(term)
  File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 489, in _validate_returning_term
    raise QueryException("You can't return from other tables")
pypika.utils.QueryException: You can't return from other tables
```
mikeengland: > Edit: Ok I see that python 3.9 test was added [here](https://github.com/kayak/pypika/commit/4f3c3bcadb63e9b4d4714305860e0315f6eb00df) and all commits since then have been failing this check so it's not unique to this change.
> 
> Some python 3.9 unit tests that are unrelated to this change are failing. Was the 3.9 check added recently? I don't see any other pull requests running this check.
> 
> ```
> ======================================================================
> ERROR: test_delete_returning_str (pypika.tests.test_deletes.PostgresDeleteTests)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/runner/work/pypika/pypika/pypika/tests/test_deletes.py", line 62, in test_delete_returning_str
>     PostgreSQLQuery.from_(self.table_abc)
>   File "/home/runner/work/pypika/pypika/pypika/utils.py", line 50, in _copy
>     result = func(self_copy, *args, **kwargs)
>   File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 476, in returning
>     self._return_field_str(term)
>   File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 518, in _return_field_str
>     self._return_field(Field(term, table=self._from[0]))
>   File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 500, in _return_field
>     self._validate_returning_term(term)
>   File "/home/runner/work/pypika/pypika/pypika/dialects.py", line 489, in _validate_returning_term
>     raise QueryException("You can't return from other tables")
> pypika.utils.QueryException: You can't return from other tables
> ```

Sorry about that - yes I added Python 3.9 to the CI and haven't had time to look at the fixing tests yet.

Patch:
diff --git a/pypika/terms.py b/pypika/terms.py
index fce885a..8935055 100644
--- a/pypika/terms.py
+++ b/pypika/terms.py
@@ -922,7 +922,6 @@ class ArithmeticExpression(Term):
     are also preserved.
     """
 
-    mul_order = [Arithmetic.mul, Arithmetic.div]
     add_order = [Arithmetic.add, Arithmetic.sub]
 
     def __init__(self, operator: Arithmetic, left: Any, right: Any, alias: Optional[str] = None) -> None:
@@ -971,16 +970,60 @@ class ArithmeticExpression(Term):
         self.left = self.left.replace_table(current_table, new_table)
         self.right = self.right.replace_table(current_table, new_table)
 
+    def left_needs_parens(self, curr_op, left_op) -> bool:
+        """
+        Returns true if the expression on the left of the current operator needs to be enclosed in parentheses.
+
+        :param current_op:
+            The current operator.
+        :param left_op:
+            The highest level operator of the left expression.
+        """
+        if left_op is None:
+            # If the left expression is a single item.
+            return False
+        if curr_op in self.add_order:
+            # If the current operator is '+' or '-'.
+            return False
+        # The current operator is '*' or '/'. If the left operator is '+' or '-', we need to add parentheses:
+        # e.g. (A + B) / ..., (A - B) / ...
+        # Otherwise, no parentheses are necessary:
+        # e.g. A * B / ..., A / B / ...
+        return left_op in self.add_order
+
+    def right_needs_parens(self, curr_op, right_op) -> bool:
+        """
+        Returns true if the expression on the right of the current operator needs to be enclosed in parentheses.
+
+        :param current_op:
+            The current operator.
+        :param right_op:
+            The highest level operator of the right expression.
+        """
+        if right_op is None:
+            # If the right expression is a single item.
+            return False
+        if curr_op == Arithmetic.add:
+            return False
+        if curr_op == Arithmetic.div:
+            return True
+        # The current operator is '*' or '-. If the right operator is '+' or '-', we need to add parentheses:
+        # e.g. ... - (A + B), ... - (A - B)
+        # Otherwise, no parentheses are necessary:
+        # e.g. ... - A / B, ... - A * B
+        return right_op in self.add_order
+
     def get_sql(self, with_alias: bool = False, **kwargs: Any) -> str:
-        is_mul = self.operator in self.mul_order
-        is_left_add, is_right_add = [
-            getattr(side, "operator", None) in self.add_order for side in [self.left, self.right]
-        ]
+        left_op, right_op = [getattr(side, "operator", None) for side in [self.left, self.right]]
 
         arithmetic_sql = "{left}{operator}{right}".format(
             operator=self.operator.value,
-            left=("({})" if is_mul and is_left_add else "{}").format(self.left.get_sql(**kwargs)),
-            right=("({})" if is_mul and is_right_add else "{}").format(self.right.get_sql(**kwargs)),
+            left=("({})" if self.left_needs_parens(self.operator, left_op) else "{}").format(
+                self.left.get_sql(**kwargs)
+            ),
+            right=("({})" if self.right_needs_parens(self.operator, right_op) else "{}").format(
+                self.right.get_sql(**kwargs)
+            ),
         )
 
         if with_alias:


Test patch:
diff --git a/pypika/tests/test_functions.py b/pypika/tests/test_functions.py
index 35ed5c6..0c0d510 100644
--- a/pypika/tests/test_functions.py
+++ b/pypika/tests/test_functions.py
@@ -171,23 +171,39 @@ class ArithmeticTests(unittest.TestCase):
     def test__complex_op_add_parentheses(self):
         q1 = Q.from_("abc").select((F("a") + 1) + (F("b") - 5))
         q2 = Q.from_(self.t).select((self.t.a + 1) + (self.t.b - 5))
+        q3 = Q.from_("abc").select((F("a") / 1) + (F("b") / 5))
+        q4 = Q.from_(self.t).select((self.t.a / 1) + (self.t.b / 5))
 
         self.assertEqual('SELECT "a"+1+"b"-5 FROM "abc"', str(q1))
         self.assertEqual('SELECT "a"+1+"b"-5 FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"/1+"b"/5 FROM "abc"', str(q3))
+        self.assertEqual('SELECT "a"/1+"b"/5 FROM "abc"', str(q4))
 
     def test__complex_op_sub_parentheses(self):
         q1 = Q.from_("abc").select((F("a") + 1) - (F("b") - 5))
         q2 = Q.from_(self.t).select((self.t.a + 1) - (self.t.b - 5))
+        q3 = Q.from_("abc").select((F("a") / 1) - (F("b") / 5))
+        q4 = Q.from_(self.t).select((self.t.a / 1) - (self.t.b / 5))
 
-        self.assertEqual('SELECT "a"+1-"b"-5 FROM "abc"', str(q1))
-        self.assertEqual('SELECT "a"+1-"b"-5 FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"+1-("b"-5) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"+1-("b"-5) FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"/1-"b"/5 FROM "abc"', str(q3))
+        self.assertEqual('SELECT "a"/1-"b"/5 FROM "abc"', str(q4))
 
     def test__complex_op_mul_parentheses(self):
         q1 = Q.from_("abc").select((F("a") + 1) * (F("b") - 5))
         q2 = Q.from_(self.t).select((self.t.a + 1) * (self.t.b - 5))
+        q3 = Q.from_("abc").select((F("a") / 1) * (F("b") - 5))
+        q4 = Q.from_(self.t).select((self.t.a / 1) * (self.t.b - 5))
+        q5 = Q.from_("abc").select((F("a") + 1) * (F("b") / 5))
+        q6 = Q.from_(self.t).select((self.t.a + 1) * (self.t.b / 5))
 
         self.assertEqual('SELECT ("a"+1)*("b"-5) FROM "abc"', str(q1))
         self.assertEqual('SELECT ("a"+1)*("b"-5) FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"/1*("b"-5) FROM "abc"', str(q3))
+        self.assertEqual('SELECT "a"/1*("b"-5) FROM "abc"', str(q4))
+        self.assertEqual('SELECT ("a"+1)*"b"/5 FROM "abc"', str(q5))
+        self.assertEqual('SELECT ("a"+1)*"b"/5 FROM "abc"', str(q6))
 
     def test__complex_op_mul_no_parentheses(self):
         q = Q.from_("abc").select(F("a") + 1 * F("b") - 5)
@@ -197,15 +213,104 @@ class ArithmeticTests(unittest.TestCase):
     def test__complex_op_div_parentheses(self):
         q1 = Q.from_("abc").select((F("a") + 1) / (F("b") - 5))
         q2 = Q.from_(self.t).select((self.t.a + 1) / (self.t.b - 5))
+        q3 = Q.from_("abc").select((F("a") / 1) / (F("b") - 5))
+        q4 = Q.from_(self.t).select((self.t.a / 1) / (self.t.b - 5))
+        q5 = Q.from_("abc").select((F("a") + 1) / (F("b") * 5))
+        q6 = Q.from_(self.t).select((self.t.a + 1) / (self.t.b * 5))
 
         self.assertEqual('SELECT ("a"+1)/("b"-5) FROM "abc"', str(q1))
         self.assertEqual('SELECT ("a"+1)/("b"-5) FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"/1/("b"-5) FROM "abc"', str(q3))
+        self.assertEqual('SELECT "a"/1/("b"-5) FROM "abc"', str(q4))
+        self.assertEqual('SELECT ("a"+1)/("b"*5) FROM "abc"', str(q5))
+        self.assertEqual('SELECT ("a"+1)/("b"*5) FROM "abc"', str(q6))
 
     def test__complex_op_div_no_parentheses(self):
         q = Q.from_("abc").select(F("a") + 1 / F("b") - 5)
 
         self.assertEqual('SELECT "a"+1/"b"-5 FROM "abc"', str(q))
 
+    def test__complex_op_exponent_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") / (F("b") ** 2))
+        q2 = Q.from_(self.t).select(self.t.a / (self.t.b ** 2))
+        q3 = Q.from_("abc").select(F("a") ** (F("b") / 2))
+        q4 = Q.from_(self.t).select(self.t.a ** (self.t.b / 2))
+        q5 = Q.from_("abc").select((F("a") ** F("b")) ** 2)
+        q6 = Q.from_(self.t).select((self.t.a ** self.t.b) ** 2)
+
+        self.assertEqual('SELECT "a"/POW("b",2) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"/POW("b",2) FROM "abc"', str(q2))
+        self.assertEqual('SELECT POW("a","b"/2) FROM "abc"', str(q3))
+        self.assertEqual('SELECT POW("a","b"/2) FROM "abc"', str(q4))
+        self.assertEqual('SELECT POW(POW("a","b"),2) FROM "abc"', str(q5))
+        self.assertEqual('SELECT POW(POW("a","b"),2) FROM "abc"', str(q6))
+
+    def test__complex_op_exponent_no_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") ** F("b") ** 2)
+        q2 = Q.from_(self.t).select(self.t.a ** self.t.b ** 2)
+        q3 = Q.from_("abc").select(F("a") / F("b") ** 2)
+        q4 = Q.from_(self.t).select(self.t.a / self.t.b ** 2)
+
+        self.assertEqual('SELECT POW("a",POW("b",2)) FROM "abc"', str(q1))
+        self.assertEqual('SELECT POW("a",POW("b",2)) FROM "abc"', str(q2))
+        self.assertEqual('SELECT "a"/POW("b",2) FROM "abc"', str(q3))
+        self.assertEqual('SELECT "a"/POW("b",2) FROM "abc"', str(q4))
+
+    def test__complex_op_function_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") / (fn.Sum(F("b")) / 2))
+        q2 = Q.from_("abc").select(self.t.a / (fn.Sum(self.t.b) / 2))
+        q3 = Q.from_("abc").select(fn.Sum(F("a") / (F("b") / 2)))
+        q4 = Q.from_("abc").select(fn.Sum(self.t.a / (self.t.b / 2)))
+
+        self.assertEqual('SELECT "a"/(SUM("b")/2) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"/(SUM("b")/2) FROM "abc"', str(q2))
+        self.assertEqual('SELECT SUM("a"/("b"/2)) FROM "abc"', str(q3))
+        self.assertEqual('SELECT SUM("a"/("b"/2)) FROM "abc"', str(q4))
+
+    def test__complex_op_modulus_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") / (F("b") % 2))
+        q2 = Q.from_(self.t).select(self.t.a / (self.t.b % 2))
+        q3 = Q.from_("abc").select(F("a") % (F("b") / 2))
+        q4 = Q.from_(self.t).select(self.t.a % (self.t.b / 2))
+        q5 = Q.from_("abc").select(F("a") % (F("b") % 2))
+        q6 = Q.from_("abc").select(self.t.a % (self.t.b % 2))
+
+        self.assertEqual('SELECT "a"/MOD("b",2) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"/MOD("b",2) FROM "abc"', str(q2))
+        self.assertEqual('SELECT MOD("a","b"/2) FROM "abc"', str(q3))
+        self.assertEqual('SELECT MOD("a","b"/2) FROM "abc"', str(q4))
+        self.assertEqual('SELECT MOD("a",MOD("b",2)) FROM "abc"', str(q5))
+        self.assertEqual('SELECT MOD("a",MOD("b",2)) FROM "abc"', str(q6))
+
+    def test__complex_op_modulus_no_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") % F("b") % 2)
+        q2 = Q.from_(self.t).select(self.t.a % self.t.b % 2)
+        q3 = Q.from_("abc").select(F("a") / F("b") % 2)
+        q4 = Q.from_(self.t).select(self.t.a / self.t.b % 2)
+
+        self.assertEqual('SELECT MOD(MOD("a","b"),2) FROM "abc"', str(q1))
+        self.assertEqual('SELECT MOD(MOD("a","b"),2) FROM "abc"', str(q2))
+        self.assertEqual('SELECT MOD("a"/"b",2) FROM "abc"', str(q3))
+        self.assertEqual('SELECT MOD("a"/"b",2) FROM "abc"', str(q4))
+
+    def test__complex_op_floor_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") / (fn.Floor(F("b")) / 2))
+        q2 = Q.from_("abc").select(self.t.a / (fn.Floor(self.t.b) / 2))
+        q3 = Q.from_("abc").select(fn.Floor(F("a") / (F("b") / 2)))
+        q4 = Q.from_("abc").select(fn.Floor(self.t.a / (self.t.b / 2)))
+
+        self.assertEqual('SELECT "a"/(FLOOR("b")/2) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"/(FLOOR("b")/2) FROM "abc"', str(q2))
+        self.assertEqual('SELECT FLOOR("a"/("b"/2)) FROM "abc"', str(q3))
+        self.assertEqual('SELECT FLOOR("a"/("b"/2)) FROM "abc"', str(q4))
+
+    def test__complex_op_nested_parentheses(self):
+        q1 = Q.from_("abc").select(F("a") / (F("b") / ((F("c") / 2))))
+        q2 = Q.from_("abc").select(self.t.a / (self.t.b / ((self.t.c / 2))))
+
+        self.assertEqual('SELECT "a"/("b"/("c"/2)) FROM "abc"', str(q1))
+        self.assertEqual('SELECT "a"/("b"/("c"/2)) FROM "abc"', str(q2))
+
     def test__arithmetic_equality(self):
         q1 = Q.from_("abc").select(F("a") / 2 == 2)
         q2 = Q.from_(self.t).select(self.t.a / 2 == 2)