rails / arel

A Relational Algebra
2.06k stars 390 forks source link

Arel::Nodes::Division does not divide first argument by second argument #488

Closed raisin closed 6 years ago

raisin commented 7 years ago

Arel::Nodes::Division.new does not appear to divide the first argument by the second argument. It merely appends "/ (second argument)" to the sql. In some cases, this is adequate, but it provides an unexpected result in other cases.

Consider the following examples:

Manually constructed SQL:

final_window_selector = "(#{Arel::Nodes::Subtraction.new(window_selector, 1).to_sql})/3 as rn" # final_window_selector.to_sql # "(row_number() OVER (PARTITION BY \"sub_table\".\"url\", \"sub_table\".\"type\" ORDER BY \"sub_table\".\"id\") - 1)/3 as rn"

SQL constructed using Arel's Arel::Nodes::Division.new operator:

final_window_selector = Arel::Nodes::Division.new(Arel::Nodes::Subtraction.new(window_selector, 1), 3).as('rn') # final_window_selector.to_sql # "row_number() OVER (PARTITION BY \"sub_table\".\"url\", \"sub_table\".\"type\" ORDER BY \"sub_table\".\"id\") - 1 / 3 AS rn"

The manually constructed SQL divides the first argument by the second argument. However, Arel::Nodes::Division.new does not divide the first argument by the second argument. It merely does "/ (second argument)". As a result, we subtract "1/3" from the (original quantity) as opposed to taking (original quantity - 1)/3.

Is this the expected behavior of Arel::Nodes::Division.new? Or, is this a bug in the SQL generation?

kbrock commented 6 years ago

@raisin Rails tends to use lots of parentheses to avoid problems like this.

You probably want to take advantage of a Grouping node

matthewd commented 6 years ago

Per #523, Arel development is moving to rails/rails.

If this issue is still relevant, please consider reopening it over there. (Note that the Rails repository does not accept feature request issues, and requires reproduction steps that rely on Active Record's documented API.)