Skip to content

Issues with precedence in generated SQL #181

Description

@foxbenjaminfox

datetime_add(t, a + b, "day") generates the SQL a + b || ' day'. Because || binds tighter than +, this is not the desired SQL, and always evaluates to NULL. Instead, it should be (a + b) || ' day'. Likewise, perhaps worse, datetime_add(t, a + b, "week") adds (a + b * 7), not (a + b) * 7, days, and similarly for datetime_add(t, a + b, "millisecond").

(date_add also has these issues.)

While investigating the above, I found some additional, though lesser, precedence issues:

is_nil(x and y) becomes x AND y IS NULL (should be (x AND y) IS NULL), is_nil(not x) becomes NOT (x) IS NULL (should be (NOT x) IS NULL), (not x) < y becomes NOT (x) < y (should be (NOT x) < y), and likewise with (not x) in [...] and x == (y in [...]) (becomes x = y IN (...), and = binds tighter than IN).

Also, within update, inc: [x: a + b] has a minor precedence bug that only matters when addition is non-associative (e.g. floating point, or integer overflow.) It becomes UPDATE ... SET x = x + a + b, which means x = (x + a) + b, but it should instead be x = x + (a + b).

Activity

  1. warmwaffles commented on Sep 20, 2026

    @warmwaffles
    Member

    I wonder if this effects postgres and mysql as well. They have similar functionalities. 🤔

  2. warmwaffles commented on Sep 20, 2026

    @warmwaffles
    Member

    @foxbenjaminfox can you take a look at #182? Maybe if you can come up with another obscure test case as well I can chuck that into the test suite as well.

  3. added a commit that references this issue on Sep 20, 2026
    b89a569
  4. foxbenjaminfox commented on Sep 20, 2026

    @foxbenjaminfox
    Author

    I went looking for similar issues in the postgres and myxql adapters, and filed an issue with what I found there: elixir-ecto/ecto_sql#754. That may be a reasonable source of additional test cases. e.g. (x == y) in [1, 2], a boolean operator on the left of an in.

    I see that #182 also parenthesized the subquery in case: that could get a test covering it too, e.g. (x + y) in subquery(...).

    Fragments as operands are another tricky issue, one which in the general case I do not expect the adapter to deal with. e.g. is_nil(fragment("? OR ?", x, y)) still becomes x OR y IS NULL, but there you could quite reasonably argue that it is the user's responsibility to add the parentheses in the fragment. But datetime_add and similar are a tougher question, because the user cannot see the implementation details—datetime_add(s.x, fragment("? + ?", y, z), "day") results in x + y || ' day', and if the user doesn't know that this is the implementation they silently get a bad result for no real reason they can see. I don't know how you want to handle that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions