fix(bigframes): parenthesize OR expressions for AND chaining in SQLGlot - #18113
fix(bigframes): parenthesize OR expressions for AND chaining in SQLGlot#18113sycai wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that OR expressions are correctly parenthesized when combined with AND operators in the SQLGlot compiler, and adds unit tests to verify the generated SQL. The reviewer pointed out that using copy=False when parenthesizing expressions can lead to AST corruption in SQLGlot due to parent pointer mutation, and recommended copying the expression instead.
| def check_and_parenthesize(expr: sge.Expression) -> sge.Expression: | ||
| if isinstance(expr, sge.Or): | ||
| return sge.paren(expr, copy=False) | ||
| return expr |
There was a problem hiding this comment.
Using copy=False when parenthesizing an expression can lead to AST corruption in SQLGlot. In SQLGlot, AST nodes maintain a reference to their parent. If expr is already part of another expression tree, wrapping it with copy=False will mutate its parent pointer to point to the new Paren node, while its original parent still retains a reference to it. This can cause unexpected side effects or bugs during AST traversal or formatting. It is safer to copy the expression (which is the default behavior of sge.paren) to prevent mutating shared nodes.
| def check_and_parenthesize(expr: sge.Expression) -> sge.Expression: | |
| if isinstance(expr, sge.Or): | |
| return sge.paren(expr, copy=False) | |
| return expr | |
| def check_and_parenthesize(expr: sge.Expression) -> sge.Expression: | |
| if isinstance(expr, sge.Or): | |
| return sge.paren(expr) | |
| return expr |
No description provided.