perf(sql): avoid extraneous operator parenthesis

[jcrist]

This PR is motivated by #9987, and is one part of decreasing the compilation overhead here.

This PR:

• Adds a benchmark for the problem. On main the SQL generation tests fail due to excessive recursion within SQLGlot
• Modifies our binary operator handling code to reduce some boilerplate
• Modifies the parenthesizing behavior of associative operators to avoid unnecessary parenthesis. Doing so unlocks an optimization within SQLGlot to generate SQL without recursion (excessive parenthesis defeat this optimization).
• Adds a snapshot test for operators to check the behavior

There are still a few cases where we excessively apply parenthesis (mostly in backends where a function is used instead of a binary operator), but this seemed good enough for now and is a nice improvement overall. After this PR the benchmarks now run successfully. I have some follow-ups to further improve performance in other places, but figured I'd split the PRs out by topic.

[cpcloud]

Nice!

What does the performance versus DuckDB raw SQL look like?

[jcrist]

from functools import reduce
from operator import add
from timeit import Timer

import pyarrow as pa
import ibis

con = ibis.duckdb.connect()

N = 100
data = pa.Table.from_pydict({f"x{i}": [1, 2, 3] for i in range(N)})
t = con.create_table("test", data)
expr = t.select(sum=reduce(add, map(t.__getitem__, t.columns)))
sql = f"SELECT {'+'.join(t.columns)} AS sum FROM test"

for name, func in [
    ("duckdb", lambda: con.raw_sql(sql).arrow()),
    ("ibis-duckdb", lambda: expr.to_pyarrow()),
]:
    n, t = Timer(func).autorange()
    print(f"{name}: {1000 * t / n:.1f} ms")

On this PR:

$ python bench.py
duckdb: 2.6 ms
ibis-duckdb: 25.9 ms

With another upcoming PR:

$  python bench.py
duckdb: 2.5 ms
ibis-duckdb: 7.2 ms

[cpcloud]

Not blocking, but I do think the .associative approach would be a bit cleaner.

[cpcloud]

Is this checking specifically for subclass overrides of this attribute?

[jcrist]

Yeah. If we did cls.BINARY_INFIX_OPS it would define the methods in every subclass (since no subclasses currently define BINARY_INFIX_OPS), clobbering any explicit visit_* methods the subclass defined. We'd need to do this for SIMPLE_OPS etc... too, except that every compiler subclass currently sets that attribute themselves with at least one override.

[cpcloud]

The bool here seems like it should be a property of the operation instead of a flag.

It's hard to tell what the boolean is referring to unless you know where it's being used.

What do you think about sticking a class attribute on the ops for this property?

[jcrist]

Not a bad idea. I'm going to merge this now but may refactor that in a follow-up.

[cpcloud]

Seems like most or all of the associative arguments can go away if you make it an op attribute.