From 8c9e8571488574d2a0c1484b3d44226af8c58c63 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Tue, 19 Dec 2023 12:24:18 -0800 Subject: [PATCH] fix: Correct division precedence without paretheses everywhere Follow up to #3969. I think this is a pretty reasonable fix. There are probably some instances where it has too many parentheses, but very few. And it uses our existing approach, doesn't require any big changes. --- prqlc/prql-compiler/src/sql/std.sql.prql | 47 ++++++++++--------- ...gration__queries__compile__arithmetic.snap | 24 +++++----- ...egration__queries__compile__pipelines.snap | 2 +- prqlc/prql-compiler/tests/integration/sql.rs | 18 ++++--- ...reference__declarations__functions__0.snap | 2 +- ...reference__declarations__functions__1.snap | 4 +- ...larations__functions__late-binding__0.snap | 6 +-- ...ions__piping-values-into-functions__0.snap | 4 +- ...ions__piping-values-into-functions__1.snap | 2 +- ...ions__piping-values-into-functions__2.snap | 2 +- ...__stdlib__README__standard-library__1.snap | 2 +- ...__stdlib__README__standard-library__2.snap | 4 +- ...ors__division-and-integer-division__0.snap | 2 +- ...ce__syntax__operators__parentheses__0.snap | 2 +- ..._syntax__operators__wrapping-lines__1.snap | 4 +- web/website/data/examples/expressions.yaml | 4 +- web/website/data/examples/functions.yaml | 2 +- 17 files changed, 71 insertions(+), 60 deletions(-) diff --git a/prqlc/prql-compiler/src/sql/std.sql.prql b/prqlc/prql-compiler/src/sql/std.sql.prql index 675371951f1a..eb8369906ad0 100644 --- a/prqlc/prql-compiler/src/sql/std.sql.prql +++ b/prqlc/prql-compiler/src/sql/std.sql.prql @@ -125,14 +125,19 @@ let read_csv = source -> s"read_csv({source:0})" let mul = l r -> null @{binding_strength=100} -let div_i = l r -> s"FLOOR(ABS({l:11} / {r:11})) * SIGN({l:0}) * SIGN({r:0})" - -# We have a simple float division by default, but it can be overridden by dialects. +let div_i = l r -> s"FLOOR(ABS({l:11} / {r:12})) * SIGN({l:0}) * SIGN({r:0})" + +# We have a simple float division by default, but it can be overridden by +# dialects. +# Note that this uses `12` for the RHS binding strength, which is one more than +# binding strength of divison. That's because we don't use associativity here, +# and so we need to make sure that the RHS is parenthesized when the binding +# strengths are equal; e.g. in `a / (b / c)`. @{binding_strength=11} -let div_f = l r -> s"{l} / ({r})" +let div_f = l r -> s"{l} / {r:12}" @{binding_strength=11} -let mod = l r -> s"{l} % ({r})" +let mod = l r -> s"{l} % {r:12}" @{binding_strength=10} let add = l r -> null @@ -176,12 +181,12 @@ let not = l -> s"NOT {l}" module ansi { @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" } module bigquery { @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" # Mathematical functions module math { @@ -196,10 +201,10 @@ module bigquery { module clickhouse { # https://clickhouse.com/docs/en/sql-reference/functions/arithmetic-functions#divide @{binding_strength=11} - let div_f = l r -> s"({l} / ({r}))" + let div_f = l r -> s"({l} / {r:12})" @{binding_strength=11} - let div_i = l r -> s"({l} DIV {r})" + let div_i = l r -> s"({l} DIV {r:12})" let regex_search = text pattern -> s"match({text:0}, {pattern:0})" @@ -210,10 +215,10 @@ module clickhouse { module duckdb { @{binding_strength=11} - let div_f = l r -> s"({l} / ({r}))" + let div_f = l r -> s"({l} / {r:12})" @{binding_strength=11} - let div_i = l r -> s"TRUNC({l:11} / {r:11})" + let div_i = l r -> s"TRUNC({l:11} / {r:12})" # String functions module string { @@ -234,7 +239,7 @@ module duckdb { module mssql { @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" # Mathematical functions module math { @@ -255,10 +260,10 @@ module mssql { module mysql { @{binding_strength=11} - let div_f = l r -> s"({l} / ({r}))" + let div_f = l r -> s"({l} / {r:12})" @{binding_strength=11} - let div_i = l r -> s"({l} DIV ({r}))" + let div_i = l r -> s"({l} DIV {r:12})" @{binding_strength=100} let mod = l r -> s"ROUND(MOD({l:0}, {r:0}))" @@ -269,10 +274,10 @@ module mysql { module postgres { @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" @{binding_strength=100} - let div_i = l r -> s"TRUNC({l:11} / ({r:11}))" + let div_i = l r -> s"TRUNC({l:11} / {r:12})" # Mathematical functions module math { @@ -293,10 +298,10 @@ module postgres { module glaredb { @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" @{binding_strength=100} - let div_i = l r -> s"TRUNC({l:11} / ({r:11}))" + let div_i = l r -> s"TRUNC({l:11} / {r:12})" # Mathematical functions module math { @@ -324,10 +329,10 @@ module sqlite { let concat_array = column -> s"GROUP_CONCAT({column:0}, '')" @{binding_strength=11} - let div_f = l r -> s"({l} * 1.0 / ({r}))" + let div_f = l r -> s"({l} * 1.0 / {r:12})" @{binding_strength=100} - let div_i = l r -> s"ROUND(ABS({l:11} / {r:11}) - 0.5) * SIGN({l:0}) * SIGN({r:0})" + let div_i = l r -> s"ROUND(ABS({l:11} / {r:12}) - 0.5) * SIGN({l:0}) * SIGN({r:0})" # String functions module string { @@ -345,5 +350,5 @@ module sqlite { module snowflake { # https://docs.snowflake.com/en/sql-reference/operators-arithmetic#division @{binding_strength=11} - let div_f = l r -> s"({l} / ({r}))" + let div_f = l r -> s"({l} / {r:12})" } diff --git a/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__arithmetic.snap b/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__arithmetic.snap index 2fa00af525dd..0a24a6955dfe 100644 --- a/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__arithmetic.snap +++ b/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__arithmetic.snap @@ -37,32 +37,32 @@ WITH table_0 AS ( ) SELECT id, - x_int / (k_int), - x_int / (k_float), - x_float / (k_int), - x_float / (k_float), + x_int / k_int, + x_int / k_float, + x_float / k_int, + x_float / k_float, FLOOR(ABS(x_int / k_int)) * SIGN(x_int) * SIGN(k_int) AS q_ii, FLOOR(ABS(x_int / k_float)) * SIGN(x_int) * SIGN(k_float) AS q_if, FLOOR(ABS(x_float / k_int)) * SIGN(x_float) * SIGN(k_int) AS q_fi, FLOOR(ABS(x_float / k_float)) * SIGN(x_float) * SIGN(k_float) AS q_ff, - x_int % (k_int) AS r_ii, - x_int % (k_float) AS r_if, - x_float % (k_int) AS r_fi, - x_float % (k_float) AS r_ff, + x_int % k_int AS r_ii, + x_int % k_float AS r_if, + x_float % k_int AS r_fi, + x_float % k_float AS r_ff, ROUND( - FLOOR(ABS(x_int / k_int)) * SIGN(x_int) * SIGN(k_int) * k_int + x_int % (k_int), + FLOOR(ABS(x_int / k_int)) * SIGN(x_int) * SIGN(k_int) * k_int + x_int % k_int, 0 ), ROUND( - FLOOR(ABS(x_int / k_float)) * SIGN(x_int) * SIGN(k_float) * k_float + x_int % (k_float), + FLOOR(ABS(x_int / k_float)) * SIGN(x_int) * SIGN(k_float) * k_float + x_int % k_float, 0 ), ROUND( - FLOOR(ABS(x_float / k_int)) * SIGN(x_float) * SIGN(k_int) * k_int + x_float % (k_int), + FLOOR(ABS(x_float / k_int)) * SIGN(x_float) * SIGN(k_int) * k_int + x_float % k_int, 0 ), ROUND( - FLOOR(ABS(x_float / k_float)) * SIGN(x_float) * SIGN(k_float) * k_float + x_float % (k_float), + FLOOR(ABS(x_float / k_float)) * SIGN(x_float) * SIGN(k_float) * k_float + x_float % k_float, 0 ) FROM diff --git a/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__pipelines.snap b/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__pipelines.snap index 1448113f04e3..1395003f97ac 100644 --- a/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__pipelines.snap +++ b/prqlc/prql-compiler/tests/integration/snapshots/integration__queries__compile__pipelines.snap @@ -12,7 +12,7 @@ WITH table_0 AS ( tracks WHERE REGEXP(name, 'Love') - AND milliseconds / (1000) / (60) BETWEEN 3 AND 4 + AND milliseconds / 1000 / 60 BETWEEN 3 AND 4 ORDER BY track_id LIMIT diff --git a/prqlc/prql-compiler/tests/integration/sql.rs b/prqlc/prql-compiler/tests/integration/sql.rs index d42fab029e51..9ec1e19277c6 100644 --- a/prqlc/prql-compiler/tests/integration/sql.rs +++ b/prqlc/prql-compiler/tests/integration/sql.rs @@ -247,18 +247,24 @@ fn json_of_test() { } #[test] -fn test_precedence_again() { +fn test_precedence_division() { assert_display_snapshot!((compile(r###" from artists derive { - p2 = x / (y * z), # needs parentheses p1 = a - (b + c), # needs parentheses + p2 = x / (y * z), # needs parentheses + np1 = x / y / z, # doesn't need parentheses + p3 = x / (y / z), # needs parentheses + np4 = (x / y) / z, # doesn't need parentheses } "###).unwrap()), @r###" SELECT *, + a - (b + c) AS p1, x / (y * z) AS p2, - a - (b + c) AS p1 + x / y / z AS np1, + x / (y / z) AS p3, + x / y / z AS np4 FROM artists "###); @@ -295,8 +301,8 @@ fn test_precedence() { "###).unwrap()), @r###" SELECT *, - (temp_f - 32) / (1.8) AS temp_c, - (temp_f - 32) / (1.8) * 9 / (5) AS temp_f, + (temp_f - 32) / 1.8 AS temp_c, + (temp_f - 32) / 1.8 * 9 / 5 AS temp_f, temp_x + 9 - 5 AS temp_z FROM x @@ -2986,7 +2992,7 @@ fn test_casting() { CAST(a AS int) + 10 AS b, CAST(a AS int) - 10 AS c, CAST(a AS float) * 10 AS d, - CAST(a AS float) / (10) AS e + CAST(a AS float) / 10 AS e FROM x "### diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__0.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__0.snap index 0f8685dd4415..4827c2f3ee1a 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__0.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__0.snap @@ -4,7 +4,7 @@ expression: "let fahrenheit_to_celsius = temp -> (temp - 32) / 1.8\n\nfrom citie --- SELECT *, - (temp_f - 32) / (1.8) AS temp_c + (temp_f - 32) / 1.8 AS temp_c FROM cities diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__1.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__1.snap index 65032f2a3b8c..a166d3f7b0df 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__1.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__1.snap @@ -4,8 +4,8 @@ expression: "let interp = low:0 high x -> (x - low) / (high - low)\n\nfrom stude --- SELECT *, - (sat_score - 0) / ((1600 - 0)) AS sat_proportion_1, - (sat_score - 0) / ((1600 - 0)) AS sat_proportion_2 + (sat_score - 0) / (1600 - 0) AS sat_proportion_1, + (sat_score - 0) / (1600 - 0) AS sat_proportion_2 FROM students diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__late-binding__0.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__late-binding__0.snap index eb21491b9aa3..1361df6fefcc 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__late-binding__0.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__late-binding__0.snap @@ -7,9 +7,9 @@ SELECT labor, overhead, cost_total, - materials / (cost_total) AS materials_share, - labor / (cost_total) AS labor_share, - overhead / (cost_total) AS overhead_share + materials / cost_total AS materials_share, + labor / cost_total AS labor_share, + overhead / cost_total AS overhead_share FROM costs diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__0.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__0.snap index a03cd65f7faa..79cf0a61ee4f 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__0.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__0.snap @@ -4,8 +4,8 @@ expression: "let interp = low:0 high x -> (x - low) / (high - low)\n\nfrom stude --- SELECT *, - (sat_score - 0) / ((1600 - 0)) AS sat_proportion_1, - (sat_score - 0) / ((1600 - 0)) AS sat_proportion_2 + (sat_score - 0) / (1600 - 0) AS sat_proportion_1, + (sat_score - 0) / (1600 - 0) AS sat_proportion_2 FROM students diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__1.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__1.snap index 22128bfe944a..3e6399378597 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__1.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__1.snap @@ -4,7 +4,7 @@ expression: "let fahrenheit_to_celsius = temp -> (temp - 32) / 1.8\n\nfrom citie --- SELECT *, - (temp_f - 32) / (1.8) AS temp_c + (temp_f - 32) / 1.8 AS temp_c FROM cities diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__2.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__2.snap index a1b2a416ce90..dbd9383eacb1 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__2.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__declarations__functions__piping-values-into-functions__2.snap @@ -4,7 +4,7 @@ expression: "let fahrenheit_to_celsius = temp -> (temp - 32) / 1.8\nlet interp = --- SELECT *, - ((temp_c - 32) / (1.8) - 0) / ((100 - 0)) AS boiling_proportion + ((temp_c - 32) / 1.8 - 0) / (100 - 0) AS boiling_proportion FROM kettles diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__1.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__1.snap index dee9c55b708b..e2c3c1e4b870 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__1.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__1.snap @@ -8,7 +8,7 @@ WITH table_0 AS ( 5 AS y ) SELECT - (x * 1.0 / (y)) AS quotient, + (x * 1.0 / y) AS quotient, ROUND(ABS(x / y) - 0.5) * SIGN(x) * SIGN(y) AS int_quotient FROM table_0 diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__2.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__2.snap index 7c12d42040f8..33be44e1fedc 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__2.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__stdlib__README__standard-library__2.snap @@ -8,8 +8,8 @@ WITH table_0 AS ( 5 AS y ) SELECT - (x / (y)) AS quotient, - (x DIV (y)) AS int_quotient + (x / y) AS quotient, + (x DIV y) AS int_quotient FROM table_0 diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__division-and-integer-division__0.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__division-and-integer-division__0.snap index 68dcf2cb730f..c1e649d9be10 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__division-and-integer-division__0.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__division-and-integer-division__0.snap @@ -13,7 +13,7 @@ WITH table_0 AS ( -2 AS b ) SELECT - (a * 1.0 / (b)) AS div_out, + (a * 1.0 / b) AS div_out, ROUND(ABS(a / b) - 0.5) * SIGN(a) * SIGN(b) AS int_div_out FROM table_0 diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__parentheses__0.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__parentheses__0.snap index 45d4cba10da5..615e42dd0781 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__parentheses__0.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__parentheses__0.snap @@ -7,7 +7,7 @@ SELECT distance BETWEEN 0 AND 20 AS is_proximate, SUM(distance) OVER () AS total_distance, MIN(COALESCE(distance, 5)) OVER () AS min_capped_distance, - distance / (40) AS travel_time, + distance / 40 AS travel_time, ROUND(distance, 1 + 1) AS distance_rounded_2_dp, distance >= 100 AS is_far, distance BETWEEN -100 AND 0, diff --git a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__wrapping-lines__1.snap b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__wrapping-lines__1.snap index 48949c925889..7330f27c202c 100644 --- a/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__wrapping-lines__1.snap +++ b/web/book/tests/documentation/snapshots/documentation__book__reference__syntax__operators__wrapping-lines__1.snap @@ -1,11 +1,11 @@ --- source: web/book/tests/documentation/book.rs -expression: "from tracks\n# This would be a really long line without being able to split it:\nselect listening_time_years = (spotify_plays + apple_music_plays + pandora_plays)\n# We can toggle between lines when developing:\n# \\ * length_seconds\n\\ * length_s\n# min hour day year\n\\ / 60 / 60 / 24 / 365\n" +expression: "from tracks\n# This would be a really long line without being able to split it:\nselect listening_time_years = (spotify_plays + apple_music_plays + pandora_plays)\n# * length_seconds\n# Actually it's `length_s` I think:\n\\ * length_s\n# min hour day year\n\\ / 60 / 60 / 24 / 365\n" --- SELECT ( spotify_plays + apple_music_plays + pandora_plays - ) * length_s / (60) / (60) / (24) / (365) AS listening_time_years + ) * length_s / 60 / 60 / 24 / 365 AS listening_time_years FROM tracks diff --git a/web/website/data/examples/expressions.yaml b/web/website/data/examples/expressions.yaml index 234078d0bc5a..32bbdf8e8013 100644 --- a/web/website/data/examples/expressions.yaml +++ b/web/website/data/examples/expressions.yaml @@ -11,8 +11,8 @@ sql: | SELECT *, started + unfinished AS finished, - (started + unfinished) / (started) AS fin_share, - (started + unfinished) / (started) / ((1 - (started + unfinished) / (started))) + (started + unfinished) / started AS fin_share, + (started + unfinished) / started / (1 - (started + unfinished) / started) AS fin_ratio FROM track_plays diff --git a/web/website/data/examples/functions.yaml b/web/website/data/examples/functions.yaml index 0c96297c46fb..965dc1f177d3 100644 --- a/web/website/data/examples/functions.yaml +++ b/web/website/data/examples/functions.yaml @@ -6,6 +6,6 @@ prql: | select temp_f = (fahrenheit_from_celsius temp_c) sql: | SELECT - temp_c * 9 / (5) + 32 AS temp_f + temp_c * 9 / 5 + 32 AS temp_f FROM weather