Skip to content

Commit ca68afa

Browse files
craig[bot]knz
craig[bot]
and
knz
committed
Merge #27255
27255: pretty: improve the simplification for binary scalar expressions r=knz a=knz Noted by @mjibson in #27225 (comment) This uses the precedence more to simplify binary expressions. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
2 parents 8252265 + de00899 commit ca68afa

File tree

4 files changed

+1013
-199
lines changed

4 files changed

+1013
-199
lines changed

‎pkg/sql/sem/tree/pretty.go‎

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -257,19 +257,41 @@ func (node *Exprs) doc(p PrettyCfg) pretty.Doc {
257257
return pretty.Join(",", d...)
258258
}
259259

260-
func (p PrettyCfg) peelBinaryOperand(e Expr, op BinaryOperator) Expr {
260+
// peelBinaryOperand conditionally (p.Simplify) removes the
261+
// parentheses around an expression. The parentheses are always
262+
// removed in the following conditions:
263+
// - if the operand is a unary operator (these are always
264+
// of higher precedence): "(-a) * b" -> "-a * b"
265+
// - if the operand is a binary operator and its precedence
266+
// is guaranteed to be higher: "(a * b) + c" -> "a * b + c"
267+
//
268+
// Additionally, iff sameLevel is set, then parentheses are removed
269+
// around any binary operator that has the same precedence level as
270+
// the parent.
271+
// sameLevel can be set:
272+
//
273+
// - for the left operand of all binary expressions, because
274+
// (in pg SQL) all binary expressions are left-associative.
275+
// This rewrites e.g. "(a + b) - c" -> "a + b - c"
276+
// and "(a - b) + c" -> "a - b + c"
277+
// - for the right operand when the parent operator is known
278+
// to be fully associative, e.g.
279+
// "a + (b - c)" -> "a + b - c" because "+" is fully assoc,
280+
// but "a - (b + c)" cannot be simplified because "-" is not fully associative.
281+
//
282+
func (p PrettyCfg) peelBinaryOperand(e Expr, sameLevel bool, parenPrio int) Expr {
261283
if !p.Simplify {
262284
return e
263285
}
264286
stripped := StripParens(e)
265287
switch te := stripped.(type) {
266288
case *BinaryExpr:
267-
parenPrio := binaryOpPrio[op]
268289
childPrio := binaryOpPrio[te.Operator]
269-
if te.Operator == op && childPrio <= parenPrio {
290+
if childPrio < parenPrio || (sameLevel && childPrio == parenPrio) {
270291
return stripped
271292
}
272-
case *UnaryExpr, *AnnotateTypeExpr, *CastExpr, *ColumnItem, *UnresolvedName:
293+
case *FuncExpr, *UnaryExpr, *AnnotateTypeExpr, *IndirectionExpr,
294+
*CastExpr, *ColumnItem, *UnresolvedName:
273295
// All these expressions have higher precedence than binary expressions.
274296
return stripped
275297
}
@@ -281,25 +303,22 @@ func (p PrettyCfg) peelBinaryOperand(e Expr, op BinaryOperator) Expr {
281303
func (node *BinaryExpr) doc(p PrettyCfg) pretty.Doc {
282304
// All the binary operators are at least left-associative.
283305
// So we can always simplify "(a OP b) OP c" to "a OP b OP c".
284-
leftOperand := p.peelBinaryOperand(node.Left, node.Operator)
285-
rightOperand := node.Right
286-
if binaryOpFullyAssoc[node.Operator] {
287-
// If the binary operator is also fully associative,
288-
// we can also simplify "a OP (b OP c)" to "a OP b OP c".
289-
rightOperand = p.peelBinaryOperand(node.Right, node.Operator)
290-
}
306+
parenPrio := binaryOpPrio[node.Operator]
307+
leftOperand := p.peelBinaryOperand(node.Left, true /*sameLevel*/, parenPrio)
308+
// If the binary operator is also fully associative,
309+
// we can also simplify "a OP (b OP c)" to "a OP b OP c".
310+
opFullyAssoc := binaryOpFullyAssoc[node.Operator]
311+
rightOperand := p.peelBinaryOperand(node.Right, opFullyAssoc, parenPrio)
312+
291313
opDoc := pretty.Text(node.Operator.String())
292314
var res pretty.Doc
293315
if !node.Operator.isPadded() {
294316
res = pretty.JoinDoc(opDoc, p.Doc(leftOperand), p.Doc(rightOperand))
295317
} else {
296318
pred := func(e Expr, recurse func(e Expr)) bool {
297319
if b, ok := e.(*BinaryExpr); ok && b.Operator == node.Operator {
298-
leftSubOperand := p.peelBinaryOperand(b.Left, node.Operator)
299-
rightSubOperand := b.Right
300-
if binaryOpFullyAssoc[node.Operator] {
301-
rightSubOperand = p.peelBinaryOperand(rightSubOperand, node.Operator)
302-
}
320+
leftSubOperand := p.peelBinaryOperand(b.Left, true /*sameLevel*/, parenPrio)
321+
rightSubOperand := p.peelBinaryOperand(b.Right, opFullyAssoc, parenPrio)
303322
recurse(leftSubOperand)
304323
recurse(rightSubOperand)
305324
return true

‎pkg/sql/sem/tree/testdata/pretty/6.golden‎

Lines changed: 68 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,10 @@ VALUES
4848
]
4949
),
5050
(
51-
(
52-
9
51+
9
52+
/ 3
53+
* 1
5354
/ 3
54-
)
55-
* (
56-
1
57-
/ 3
58-
)
5955
),
6056
(
6157
2.0
@@ -115,26 +111,26 @@ VALUES
115111
]
116112
),
117113
(
118-
(
119-
9
114+
9
115+
/ 3
116+
* 1
120117
/ 3
121-
)
122-
* (
123-
1
124-
/ 3
125-
)
126118
),
127119
(2.0),
128120
(
129121
2.4
130122
+ 4.6
131123
)
132124

133-
14:
134-
--------------
125+
13:
126+
-------------
135127
INSERT
136128
INTO
137-
kv2 (k, v)
129+
kv2
130+
(
131+
k,
132+
v
133+
)
138134
VALUES
139135
(
140136
'a',
@@ -176,30 +172,38 @@ VALUES
176172
]
177173
),
178174
(
179-
(
180-
9
175+
9 / 3
176+
* 1
181177
/ 3
182-
)
183-
* (
184-
1
185-
/ 3
186-
)
187178
),
188179
(2.0),
189180
(
190181
2.4
191182
+ 4.6
192183
)
193184

194-
15:
195-
---------------
185+
14:
186+
--------------
196187
INSERT
197-
INTO kv2 (k, v)
188+
INTO
189+
kv2 (k, v)
198190
VALUES
199-
('a', 'b'),
200-
('c', 'd'),
201-
('e', 'f'),
202-
('f', 'g'),
191+
(
192+
'a',
193+
'b'
194+
),
195+
(
196+
'c',
197+
'd'
198+
),
199+
(
200+
'e',
201+
'f'
202+
),
203+
(
204+
'f',
205+
'g'
206+
),
203207
(
204208
ARRAY[
205209
NULL::INT
@@ -224,17 +228,18 @@ VALUES
224228
]
225229
),
226230
(
227-
(9 / 3)
228-
* (
229-
1
230-
/ 3
231-
)
231+
9 / 3
232+
* 1
233+
/ 3
232234
),
233235
(2.0),
234-
(2.4 + 4.6)
236+
(
237+
2.4
238+
+ 4.6
239+
)
235240

236-
17:
237-
-----------------
241+
15:
242+
---------------
238243
INSERT
239244
INTO kv2 (k, v)
240245
VALUES
@@ -266,22 +271,26 @@ VALUES
266271
]
267272
),
268273
(
269-
(9 / 3)
270-
* (1 / 3)
274+
9 / 3
275+
* 1 / 3
271276
),
272277
(2.0),
273278
(2.4 + 4.6)
274279

275-
23:
276-
-----------------------
280+
20:
281+
--------------------
277282
INSERT
278283
INTO kv2 (k, v)
279284
VALUES
280285
('a', 'b'),
281286
('c', 'd'),
282287
('e', 'f'),
283288
('f', 'g'),
284-
(ARRAY[NULL::INT]),
289+
(
290+
ARRAY[
291+
NULL::INT
292+
]
293+
),
285294
(
286295
ARRAY[
287296
NULL::INT,
@@ -300,15 +309,12 @@ VALUES
300309
NULL::INT
301310
]
302311
),
303-
(
304-
(9 / 3)
305-
* (1 / 3)
306-
),
312+
(9 / 3 * 1 / 3),
307313
(2.0),
308314
(2.4 + 4.6)
309315

310-
24:
311-
------------------------
316+
23:
317+
-----------------------
312318
INSERT
313319
INTO kv2 (k, v)
314320
VALUES
@@ -335,7 +341,7 @@ VALUES
335341
NULL::INT
336342
]
337343
),
338-
((9 / 3) * (1 / 3)),
344+
(9 / 3 * 1 / 3),
339345
(2.0),
340346
(2.4 + 4.6)
341347

@@ -357,7 +363,7 @@ VALUES
357363
NULL::INT
358364
]
359365
),
360-
((9 / 3) * (1 / 3)),
366+
(9 / 3 * 1 / 3),
361367
(2.0),
362368
(2.4 + 4.6)
363369

@@ -374,23 +380,23 @@ VALUES
374380
(ARRAY[NULL::INT, 1]),
375381
(ARRAY[1, NULL::INT]),
376382
(ARRAY[NULL::INT, NULL::INT]),
377-
((9 / 3) * (1 / 3)),
383+
(9 / 3 * 1 / 3),
378384
(2.0),
379385
(2.4 + 4.6)
380386

381-
188:
382-
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
387+
184:
388+
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
383389
INSERT
384390
INTO kv2 (k, v)
385391
VALUES
386-
('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), ((9 / 3) * (1 / 3)), (2.0), (2.4 + 4.6)
392+
('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), (9 / 3 * 1 / 3), (2.0), (2.4 + 4.6)
387393

388-
191:
389-
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
394+
187:
395+
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
390396
INSERT
391397
INTO kv2 (k, v)
392-
VALUES ('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), ((9 / 3) * (1 / 3)), (2.0), (2.4 + 4.6)
398+
VALUES ('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), (9 / 3 * 1 / 3), (2.0), (2.4 + 4.6)
393399

394-
214:
395-
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
396-
INSERT INTO kv2 (k, v) VALUES ('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), ((9 / 3) * (1 / 3)), (2.0), (2.4 + 4.6)
400+
210:
401+
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
402+
INSERT INTO kv2 (k, v) VALUES ('a', 'b'), ('c', 'd'), ('e', 'f'), ('f', 'g'), (ARRAY[NULL::INT]), (ARRAY[NULL::INT, 1]), (ARRAY[1, NULL::INT]), (ARRAY[NULL::INT, NULL::INT]), (9 / 3 * 1 / 3), (2.0), (2.4 + 4.6)

0 commit comments

Comments
 (0)