The great Box-ification experiment

[LucaCappelletti94]

As @xitep pointed out back in #2218 the Statement enum is very large, back then about 2KB, and now has surpassed 3KB on main because CreateTable and the other large payloads are stored inline. Every slot of the Vec<Statement> the parser returns pays that size, and so does every query body, since SetExpr holds a Statement inline for Insert, Update, Delete and Merge and each Box<SetExpr> therefore allocates 3440 bytes, SELECT 1 included. Parsing a script of 12K short statements retains 109 MB.

This PR puts 139 of the 140 data-carrying variants behind a Box. Statement drops to 24 bytes and SetExpr to 32, the large_enum_variant allow is gone and a test keeps Statement at 24 bytes or less.

Retained memory falls 54% on the 12K statements script, parse time on statement-heavy scripts drops ~10%, and single-statement parses are about 7% faster.

Now, I understand that reviewing this as a +5k/-5k line is nearly impossible, so if you agree that it is actually desirable to proceed with this non-trivial refactoring which will definitely break code downstream (I checked that in DataFusion 23 of 35 sites need an update, all in datafusion/sql), I will proceed to turn it into a very long stacked PR which can then be processed a statement at the time.

If you reckon that it is an excessive API break and the gains are insufficient, I will close the PR.

Refactoring drafted with Opus 5.5

cc @iffyio @alamb

[github-actions]

No dialect label was applied because the title does not name a SQL dialect.

If this change targets one or more dialects, edit the title to <Dialect>[, <Dialect>]: <description>, add a Dialects: <Dialect>[, <Dialect>] line to the description, or reply with one. Reply Dialects: none if the change is dialect-agnostic.

Changed files touch BigQuery, ClickHouse, Databricks, DuckDB, Hive, MySQL, Oracle, PostgreSQL, Snowflake, SQL Server, SQLite.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 71.75926% with 122 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.14%. Comparing base (560a13c) to head (57c822b).

Files with missing lines	Patch %	Lines
src/ast/spans.rs	4.83%	118 Missing ⚠️
src/parser/mod.rs	98.10%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2592      +/-   ##
==========================================
+ Coverage   81.09%   81.14%   +0.05%     
==========================================
  Files          42       42              
  Lines       33684    33881     +197     
  Branches    33684    33881     +197     
==========================================
+ Hits        27316    27493     +177     
- Misses       2798     2818      +20     
  Partials     3570     3570              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alamb]

I haven't looked at the PR, but the basic idea sounds reasonable

COncerns:

1. What will this do to parsing speed (every Box is another allocation) -- we should probably measure that
2. How will downstream crates need to be updated (can they still use match ... type statements)

I think as long as we have benchmarks that show this doesn't cause (too much) performance regression and a clear upgrade guide it would be good

[LucaCappelletti94]

I haven't looked at the PR, but the basic idea sounds reasonable

The gist is what you imagine, box-ifying all entries in the statements enum, fixing the associated code. My goal here was primarily to measure how big of an impact the change would have.

2. How will downstream crates need to be updated (can they still use `match ...` type statements)

This is most likely to be the pain point:

https://github.com/LucaCappelletti94/sqlparser-rs/blob/57c822b1b33a9612a016a45d40692cc39afdb40d/tests/sqlparser_postgres.rs#L347-L361

1. What will this do to parsing speed (every Box is another allocation) -- we should probably measure that

I was expecting a slow down, but the size of the enum is currently so massive in my 12k statements corpus it accelerated by 10%. Out of caution, I will prepare another benchmark based on https://sql-ast-benchmark.luca.phd/ corpus and get back to you with those results.

[LucaCappelletti94]

Downstream code can keep destructuring payloads on stable Rust if Statement gets one accessor per variant that returns the unboxed payload, generated by a small macro inside the crate, but yeah that also feels unclean and heavily sub-optimal.

impl Statement {
    pub fn as_create_table(&self) -> Option<&CreateTable> {
        match self {
            Self::CreateTable(create_table) => Some(create_table),
            _ => None,
        }
    }

    pub fn into_create_table(self) -> Option<CreateTable> {
        match self {
            Self::CreateTable(create_table) => Some(*create_table),
            _ => None,
        }
    }
}

match pg_and_generic().one_statement_parses_to(sql, "").into_create_table() {
    Some(CreateTable {
        name,
        columns,
        constraints,
        table_options,
        if_not_exists: false,
        external: false,
        file_format: None,
        location: None,
        ..
    }) => { /* assertions unchanged */ }
    _ => unreachable!(),
}

A match over several variants at once still binds the payload, as in Statement::Insert(insert) => insert.table, and field access reads through the Box transparently. On nightly, there is the experimental #![feature(deref_patterns)] from rust-lang/rust#87121 but who knows when that could stabilize.

An idea that could be viable, is to create a macro that generates BOTH the Statement enum, and a BoxedStatement enum, and then we add a generic to the parser chain, which would allow users to pick and choose which statement they want to deserialize the SQL into - now that I think of it for a second longer, this last idea seems very interesting to me as we could use it to make user specify at compile time the pool of possible variants that they accept out of the parser, possibly accelerating the code and certainly cleaning it.

I will play with this last idea and tell you whether anything nice pans out of it. The idea of a compile-time typed restricted parser output feels very Rust-y to me, and I suspect it could be done.