operator precedence must take associativity into consideration

go/vt/sqlparser/ast.go

@@ -1564,17 +1564,17 @@ func (node Exprs) Format(buf *TrackedBuffer) {
 
 // Format formats the node.
 func (node *AndExpr) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v and %v", node.Left, node.Right)
+ buf.astPrintf(node, "%l and %r", node.Left, node.Right)
 }
 
 // Format formats the node.
 func (node *OrExpr) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v or %v", node.Left, node.Right)
+ buf.astPrintf(node, "%l or %r", node.Left, node.Right)
 }
 
 // Format formats the node.
 func (node *XorExpr) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v xor %v", node.Left, node.Right)
+ buf.astPrintf(node, "%l xor %r", node.Left, node.Right)
 }
 
 // Format formats the node.
@@ -1584,15 +1584,15 @@ func (node *NotExpr) Format(buf *TrackedBuffer) {
 
 // Format formats the node.
 func (node *ComparisonExpr) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v %s %v", node.Left, node.Operator, node.Right)
+ buf.astPrintf(node, "%l %s %r", node.Left, node.Operator, node.Right)
  if node.Escape != nil {
  buf.astPrintf(node, " escape %v", node.Escape)
  }
 }
 
 // Format formats the node.
 func (node *RangeCond) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v %s %v and %v", node.Left, node.Operator, node.From, node.To)
+ buf.astPrintf(node, "%v %s %l and %r", node.Left, node.Operator, node.From, node.To)
 }
 
 // Format formats the node.
@@ -1665,7 +1665,7 @@ func (node ListArg) Format(buf *TrackedBuffer) {
 
 // Format formats the node.
 func (node *BinaryExpr) Format(buf *TrackedBuffer) {
- buf.astPrintf(node, "%v %s %v", node.Left, node.Operator, node.Right)
+ buf.astPrintf(node, "%l %s %r", node.Left, node.Operator, node.Right)
 }
 
 // Format formats the node.

go/vt/sqlparser/precedence_test.go

@@ -142,6 +142,14 @@ func TestParens(t *testing.T) {
  {in: "(a | b) between (5) and (7)", expected: "a | b between 5 and 7"},
  {in: "(a and b) between (5) and (7)", expected: "(a and b) between 5 and 7"},
  {in: "(true is true) is null", expected: "(true is true) is null"},
+ {in: "3 * (100 div 3)", expected: "3 * (100 div 3)"},
+ {in: "100 div 2 div 2", expected: "100 div 2 div 2"},
+ {in: "100 div (2 div 2)", expected: "100 div (2 div 2)"},
+ {in: "(100 div 2) div 2", expected: "100 div 2 div 2"},
+ {in: "((((((1000))))))", expected: "1000"},
+ {in: "100 - (50 + 10)", expected: "100 - (50 + 10)"},
+ {in: "100 - 50 + 10", expected: "100 - 50 + 10"},
+ {in: "true and (true and true)", expected: "true and (true and true)"},
  }
 
  for _, tc := range tests {

go/vt/sqlparser/tracked_buffer.go

@@ -56,7 +56,9 @@ func (buf *TrackedBuffer) WriteNode(node SQLNode) *TrackedBuffer {
 // Myprintf mimics fmt.Fprintf(buf, ...), but limited to Node(%v),
 // Node.Value(%s) and string(%s). It also allows a %a for a value argument, in
 // which case it adds tracking info for future substitutions.
-// It adds parens as needed to follow precedence rules when printing expressions
+// It adds parens as needed to follow precedence rules when printing expressions.
+// To handle parens correctly for left associative binary operators,
+// use %l and %r to tell the TrackedBuffer which value is on the LHS and RHS
 //
 // The name must be something other than the usual Printf() to avoid "go vet"
 // warnings due to our custom format specifiers.
@@ -87,7 +89,8 @@ func (buf *TrackedBuffer) astPrintf(currentNode SQLNode, format string, values .
  break
  }
  i++ // '%'
- switch format[i] {
+ token := format[i]
+ switch token {
  case 'c':
  switch v := values[fieldnum].(type) {
  case byte:
@@ -106,19 +109,19 @@ func (buf *TrackedBuffer) astPrintf(currentNode SQLNode, format string, values .
  default:
  panic(fmt.Sprintf("unexpected TrackedBuffer type %T", v))
  }
- case 'v':
+ case 'l', 'r', 'v':
+ left := token != 'r'
  value := values[fieldnum]
  expr := getExpressionForParensEval(checkParens, value)
 
- if expr != nil { //
- needParens := needParens(currentExpr, expr)
+ if expr == nil {
+ buf.formatter(value.(SQLNode))
+ } else {
+ needParens := needParens(currentExpr, expr, left)
  buf.printIf(needParens, "(")
  buf.formatter(expr)
  buf.printIf(needParens, ")")
- } else {
- buf.formatter(value.(SQLNode))
  }
-
  case 'a':
  buf.WriteArg(values[fieldnum].(string))
  default:
@@ -153,15 +156,34 @@ func (buf *TrackedBuffer) formatter(node SQLNode) {
  }
 }
 
-func needParens(op, val Expr) bool {
+//needParens says if we need a parenthesis
+// op is the operator we are printing
+// val is the value we are checking if we need parens around or not
+// left let's us know if the value is on the lhs or rhs of the operator
+func needParens(op, val Expr, left bool) bool {
+ // Values are atomic and never need parens
+ if IsValue(val) {
+ return false
+ }
+
  if areBothISExpr(op, val) {
  return true
  }
 
  opBinding := precedenceFor(op)
  valBinding := precedenceFor(val)
 
- return !(opBinding == Syntactic || valBinding == Syntactic) && valBinding > opBinding
+ if opBinding == Syntactic || valBinding == Syntactic {
+ return false
+ }
+
+ if left {
+ // for left associative operators, if the value is to the left of the operator,
+ // we only need parens if the order is higher for the value expression
+ return valBinding > opBinding
+ }
+
+ return valBinding >= opBinding
 }
 
 func areBothISExpr(op Expr, val Expr) bool {