fix: add parentheses to nested binary expressions in Display and SqlDisplay

[AndreaBozzo]

Which issue does this PR close?

Closes #16054

Rationale for this change

Binary expressions like (1+2)*3 were displayed incorrectly as Int64(1) + Int64(2) * Int64(3) in Display and SqlDisplay output, without parentheses to preserve operator precedence. This is misleading because it does not reflect the actual expression tree structure.

What changes are included in this PR?

• Modified Display for BinaryExpr to always wrap nested binary expressions in parentheses (following DuckDB's approach). This simplifies the previous precedence-based logic and avoids ambiguity.
• Added the same parenthesization to SqlDisplay for SQL generation output.
• Added a comment to SchemaDisplay clarifying that it intentionally does not add parentheses, because schema names must remain stable for field lookups in optimizers like common_subexpr_eliminate.
• Updated sqllogictests and optimizer integration test snapshots to reflect the new output format.

What this PR does NOT change

• SchemaDisplay is intentionally unchanged. Schema names must remain stable because they are used as field lookup keys in optimizers (e.g. common_subexpr_eliminate). Changing them would break field resolution. This was initially included and then reverted after discovering it caused optimizer failures.
• CLI column headers are not affected. When you run e.g. select (1+2)*3, the column header uses SchemaDisplay, so it will still show Int64(1) + Int64(2) * Int64(3) without parentheses. Fixing the CLI column headers would require changes to SchemaDisplay, which is out of scope for the reasons above.

Known limitation: excessive parentheses for associative operators

Because our expression tree represents a OR b OR c as nested BinaryExpr nodes ((a OR b) OR c), always-parenthesizing produces (a OR b) OR c instead of the flat a OR b OR c. DuckDB avoids this by using a flat ConjunctionExpression with a list of children, so they get (a OR b OR c). This is a structural limitation of how DataFusion represents expressions, not something that can be fixed in the Display layer alone.

Pending: datafusion-testing submodule update

The "Run sqllogictests with the sqlite test suite" CI check fails because 4 files in the datafusion-testing submodule contain error message regex patterns that need updating to match the new parenthesized output. The affected files are:

• data/sqlite/index/random/10/slt_good_0.slt (5 errors)
• data/sqlite/index/random/10/slt_good_13.slt (10 errors)
• data/sqlite/index/random/10/slt_good_4.slt (5 errors)
• data/sqlite/index/random/10/slt_good_6.slt (5 errors)

A separate PR on datafusion-testing will be needed to update these regex patterns, and then this PR's submodule pointer will need to be updated to point to the new commit.

How are these changes tested?

• New unit test test_binary_expr_display_with_parentheses covering:
  • Arithmetic expressions: (1+2)*3
  • Right-nested expressions: 1+(2*3)
  • Logical expressions: (a OR b) AND c
• Updated ~44 sqllogictest files and optimizer integration test snapshots
• All CI checks pass except the sqlite test suite (see "Pending" section above)

Are these changes safe?

Yes. The Display change only affects human-readable plan output (e.g. EXPLAIN plans, debug logging). SchemaDisplay (used for field lookups and schema stability) is intentionally unchanged. SqlDisplay gains parentheses which makes generated SQL unambiguous.

---

[AndreaBozzo]

-- deleted, was a progress meter for myself --

[AndreaBozzo]

Updated after rebase on main.

Note: There are also 3 files in the datafusion-testing submodule that need updating for the sqlite test suite:

• data/sqlite/index/random/10/slt_good_13.slt
• data/sqlite/index/random/10/slt_good_4.slt
• data/sqlite/index/random/10/slt_good_6.slt

These contain error message patterns that now include the additional parentheses (e.g., (- cor0.col4) + Int64(26) → ((- cor0.col4) + Int64(26))).

I'll create a separate PR on datafusion-testing once this PR gets reviewed, but its safe to say that the longer it takes, the harder is gonna get, very open on getting mantainers guidence now

[Jefffrey]

Is there a way to omit these parentheses in cases like this? Since it's all OR

[AndreaBozzo]

I'll try to explain what i found but my current understanding might be limited.

Seems like DuckDB also always adds parentheses, see conjunction_expression.hpp#L42-L48

they represent a OR b OR c as a flat list of children (ConjunctionExpression), they get (a OR b OR c) while our nested BinaryExpr tree produces ((a OR b) OR c).

Its the reason why i think we should discuss this further as its kinda delicate, and more people should take a look

Thank you for your time mr.Vo

[Jefffrey]

That makes sense; I suppose it's more a limitation on how we represent OR rather than on the display code in this PR 👍

[Jefffrey]

I'm a little confused by the intent of this PR; the body seems to imply it fixes cases where parentheses aren't being added to preserve precedence, but I'm fairly sure we already have this code? Especially judging by the discussion on #16054

Moreover the example in the PR body is incorrect with the behaviour of this PR:

After:

> select (1+2)*3;
+------------------------------------+
| (Int64(1) + Int64(2)) * Int64(3)   |
+------------------------------------+

I tried out this PR with cli and the parentheses aren't added because this is a schema name. And there is a test added in this PR contradicting this statement.

Could you help clarify what exactly this PR is achieving that we don't have on main?

[AndreaBozzo]

This originally contained changes to both 'Display' and 'SchemaDisplay', but then i had to revert schema display as It seems It Needed to be stabile for optimizer field lookup.

Unfortunately this way It doesn't solve the original Issue pointed in #16054, the parentheses won't appear in CLI, which Is also part of why i asked for direct help.

While It should still provide an improvement for SQL generation and debug.. its not shaping the way i initially thought/wanted.

[Jefffrey]

Would you be able to update the title & PR body to more accurately represent what changes are being made here? I'm still not exactly clear what the intended changes are which makes it harder to review

[AndreaBozzo]

Hi @Jefffrey, thanks for the feedback. I've updated the PR title and body to reflect the current state of the changes after the SchemaDisplay revert, and also added everything i could find about.

Given the scope creep of ~44 sqllogictest updates, 4 datafusion-testing submodule files needing a separate PR, and the known limitation with excessive parentheses on associative operators, i'm wondering whether this change is worth the churn on its own, or if it would be better to revisit this as part of a broader effort to handle parentheses consistently across all display modes.

Happy to hear your thoughts on whether to proceed or hold off, and if its the second case, i apologize for wasting someone else time, the intent behind was genuine.