Skip to content

fix(optimizer): enhance simplify_parens to handle additional parent t…#7339

Open
MuSilk wants to merge 1 commit intotobymao:mainfrom
MuSilk:simplify_parens
Open

fix(optimizer): enhance simplify_parens to handle additional parent t…#7339
MuSilk wants to merge 1 commit intotobymao:mainfrom
MuSilk:simplify_parens

Conversation

@MuSilk
Copy link

@MuSilk MuSilk commented Mar 19, 2026

Fixes #7338

Problem

The sqlglot.optimizer.simplify.simplify_parens() function incorrectly removes parentheses around a comparison (exp.Predicate) when it is used as an operand in an arithmetic expression.
In dialects like MySQL, boolean results are treated as integers (0 or 1). Consequently, an expression like a - (b < c) is valid and meaningful. However, the current optimizer logic only checks if the parent is exp.Neg before preserving parentheses for predicates. It fails to account for other binary arithmetic operators (Add, Sub, Mul, etc.).
This leads to incorrect SQL generation where operator precedence is altered, changing the logic of the query.
Reproduction
Input SQL:

SELECT * FROM A WHERE a - (b < c) >= 0

Current Incorrect Output:

SELECT * FROM A WHERE a - b < c >= 0

-- Parentheses around (b < c) are dropped incorrectly
-- This changes precedence: subtraction happens before comparison
SELECT * FROM A WHERE a - b < c >= 0
Note: a - b < c >= 0 is logically equivalent to (a - b) < c AND c >= 0 (depending on dialect chaining rules), which is completely different from a - (result_of_comparison) >= 0.

Root Cause

In sqlglot/optimizer/simplify.py, the simplify_parens function contains a condition:

isinstance(this, exp.Predicate) and not (parent_is_predicate or isinstance(parent, exp.Neg))

This logic assumes that only Neg requires parentheses around a predicate, ignoring binary arithmetic operators where the predicate acts as a numeric term.

Solution

Expand the check to include all binary arithmetic operators that have higher or equal precedence implications when a predicate is cast to a number. The fix adds exp.Add, exp.Sub, exp.Mul, exp.Div, exp.Mod, and exp.Pow to the exclusion list.

Copy link
Collaborator

@VaggelisD VaggelisD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @MuSilk, thank you for the fix! I could repro this in MySQL:

# Parentheses
mysql> SELECT 0 - (1 < 2) >= 0;
0


# Simplified
mysql> SELECT 0 - 1 < 2 >= 0;
1

Leaving a couple of comments:

parent_is_predicate
or isinstance(
parent,
(exp.Neg, exp.Add, exp.Sub, exp.Mul, exp.Div, exp.Mod, exp.Pow),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should instance-check against exp.Binary instead of each one individually e.g isinstance(parent, exp.Neg, exp.Binary)

or (
isinstance(this, exp.Predicate)
and not (parent_is_predicate or isinstance(parent, exp.Neg))
and not (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add tests in simplify.sql that exercise a couple of these paths? Even something simple as SELECT * FROM A WHERE a - (b < c) >= 0 AND a + (b < c) >= 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

optimize: simplify_parens removes parentheses around predicates in arithmetic context (MySQL), changing semantics

2 participants