What happened? Attempting to do math fails at BEMDAS in even some

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

Parentheses omitted from sql output in non-commutative operations about prql HOT 8 CLOSED

syko commented on September 21, 2024

Parentheses omitted from sql output in non-commutative operations

from prql.

Comments (8)

richb-hanover commented on September 21, 2024

Good bug report. Thanks.

This is a bug in the deployed Playground, it's fixed in main and could be rolled into a new release fairly soon

NB I use the Dev Container to stay up to date with the latest, and to test this...

from prql.

PrettyWood commented on September 21, 2024

@richb-hanover The bug is still here on main (see #3968)
(a and b were indeed good thanks to #3846 but not c and d)

from prql.

richb-hanover commented on September 21, 2024

Yikes! You're right. The addition/subtraction paren's are correct, multiply/divide are not... (See generated SQL below)

@syko Sorry for the misinformation...

SELECT
  *,
  10 - (10 - 2) AS a,
  10 - (10 + 2) AS b,
  10 / 10 / 2 AS c,
  10 / 10 * 2 AS d
FROM
  foo

-- Generated by PRQL compiler version:0.10.2 (https://prql-lang.org)

from prql.

max-sixty commented on September 21, 2024

Eeek, I'm looking. Sorry for this...

from prql.

max-sixty commented on September 21, 2024

OK, so the root of the issue is that for division, we're using s-strings

prql/prqlc/prql-compiler/src/sql/std.sql.prql

Lines 271 to 272 in 192be60

 @{binding_strength=11} 

 let div_f = l r -> s"({l} * 1.0 / {r})"

, which don't have any associativity. For Minus, the compiler implements a fairly robust approach in rust

prql/prqlc/prql-compiler/src/sql/gen_expr.rs

Lines 802 to 846 in cc8adde

 /// For an operation represented as `a child b` with a surrounding parent 

 /// operation (e.g., `(a child b) parent c` or `a parent (b child c)`): 

 /// 

 /// 1. When the child operator has higher precedence than the parent, 

 /// parentheses *are not* required. 

 /// 

 /// 2. When the child operator has lower precedence than the parent, 

 /// parentheses *are* required. 

 /// 

 /// 3. When the child and parent operators have the same precedence, the child 

 /// is on the {left,right} and the parent is {left,right} associative, 

 /// parentheses are not required. Some examples of when parentheses are not required: 

 /// - `(a - b) - c` & `(a + b) - c` — as opposed to `a - (b - c)` 

 /// - `a + (b - c)` & `a + (b + c)` — as opposed to `a - (b + c)` & `a - (b - c)` 

 ///  

 ///  

 // 

 // If it were possible to evaluate this with less context that would be 

 // preferable, but it's not clear how to do that. (For example, even if we 

 // passed a reference to the parent, that still wouldn't tell us whether the 

 // child were on the left or the right, which is required...) 

 // 

 // Note that the code is deliberately somewhat verbose. While it could instead 

 // be a neat single expression, it was quite difficult to work through, so 

 // please do not make the code terser without being confident that it's easier 

 // to understand. 

 fn needs_parentheses( 

 expr: &ExprOrSource, 

 is_left: bool, 

 parent_strength: i32, 

 parent_associativity: Associativity, 

 ) -> bool { 

 let rule_3a = matches!(parent_associativity, Associativity::Both); 

 let rule_3b_left = is_left && parent_associativity.left_associative(); 

 let rule_3b_right = !is_left && parent_associativity.right_associative(); 

 match expr.binding_strength().cmp(&parent_strength) { 

 // Rule 1 

 Ordering::Greater => false, 

 // Rule 2 

 Ordering::Less => true, 

 // Rule 3 

 Ordering::Equal => !(rule_3a || rule_3b_left || rule_3b_right), 

 } 

 }

I think we can patch this by putting parentheses everywhere — #3969 — but I think we might need to rethink whether we can use s-strings as extensively as we have been for these operators. We could add an annotation for associativity.

from prql.

richb-hanover commented on September 21, 2024

@max-sixty I tested this in my Dev Container and it works as expected. Thanks.

... but I think we might need to rethink whether we can use s-strings as extensively as we have been for these operators.

Would you give an example or two of where an s-string could run into difficulty?

from prql.

max-sixty commented on September 21, 2024

Would you give an example or two of where an s-string could run into difficulty?

I adjusted the approach in #3970, so it's much more reasonable now — check that one out...

from prql.

syko commented on September 21, 2024

Nice! That's some fast-action bugfixing 👏

from prql.

Parentheses omitted from sql output in non-commutative operations about prql HOT 8 CLOSED

Comments (8)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	@{binding_strength=11}
	let div_f = l r -> s"({l} * 1.0 / {r})"

	/// For an operation represented as `a child b` with a surrounding parent
	/// operation (e.g., `(a child b) parent c` or `a parent (b child c)`):
	///
	/// 1. When the child operator has higher precedence than the parent,
	/// parentheses are not required.
	///
	/// 2. When the child operator has lower precedence than the parent,
	/// parentheses are required.
	///
	/// 3. When the child and parent operators have the same precedence, the child
	/// is on the {left,right} and the parent is {left,right} associative,
	/// parentheses are not required. Some examples of when parentheses are not required:
	/// - `(a - b) - c` & `(a + b) - c` — as opposed to `a - (b - c)`
	/// - `a + (b - c)` & `a + (b + c)` — as opposed to `a - (b + c)` & `a - (b - c)`
	///
	///
	//
	// If it were possible to evaluate this with less context that would be
	// preferable, but it's not clear how to do that. (For example, even if we
	// passed a reference to the parent, that still wouldn't tell us whether the
	// child were on the left or the right, which is required...)
	//
	// Note that the code is deliberately somewhat verbose. While it could instead
	// be a neat single expression, it was quite difficult to work through, so
	// please do not make the code terser without being confident that it's easier
	// to understand.
	fn needs_parentheses(
	expr: &ExprOrSource,
	is_left: bool,
	parent_strength: i32,
	parent_associativity: Associativity,
	) -> bool {
	let rule_3a = matches!(parent_associativity, Associativity::Both);
	let rule_3b_left = is_left && parent_associativity.left_associative();
	let rule_3b_right = !is_left && parent_associativity.right_associative();

	match expr.binding_strength().cmp(&parent_strength) {
	// Rule 1
	Ordering::Greater => false,
	// Rule 2
	Ordering::Less => true,
	// Rule 3
	Ordering::Equal => !(rule_3a \|\| rule_3b_left \|\| rule_3b_right),
	}
	}