You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "LucaCappelletti94 (via GitHub)" <gi...@apache.org> on 2026/09/26 14:39:08 UTC
[PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
LucaCappelletti94 opened a new pull request, #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5961692989
thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "LucaCappelletti94 (via GitHub)" <gi...@apache.org>.
LucaCappelletti94 commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5960015055
> So what is the next step here? WOuld you like me to try a PR in datafusion to see what this change would look like downstream?
My expectation is that any generic-based approach refactoring (and I do not see other approaches that would not make it extremely tedious not being able to match & unpack downstream) would go hand-in-hand with the trait refactoring, but that is a large amount of work in itself. I would aim to explore this more in details sometimes in November, keeping for this month's target to complete the refactoring and close as many fuzzer gaps as possible before thinking as such extensive refactoring.
> Do we have any raw parsing benchmarks here in this repo?
Insufficient in my opinion for any generalizable claims. We need to prepare a suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "LucaCappelletti94 (via GitHub)" <gi...@apache.org>.
LucaCappelletti94 commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5853553828
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5959773065
So what is the next step here? WOuld you like me to try a PR in datafusion to see what this change would look like downstream?
Do we have any raw parsing benchmarks here in this repo?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "LucaCappelletti94 (via GitHub)" <gi...@apache.org>.
LucaCappelletti94 commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5853833221
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.
```rust
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,
}
}
}
```
```rust
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5849471684
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5847146984
## [Codecov](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `71.75926%` with `122 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.14%. Comparing base ([`560a13c`](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/commit/560a13c32b6c3b9f1a202d6b9a9020c016bbf7be?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c83b9d5`](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/commit/c83b9d55744c5f9ec1b2e1fa779036ca6fbb2400?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [src/ast/spans.rs](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?src=pr&el=tree&filepath=src%2Fast%2Fspans.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL2FzdC9zcGFucy5ycw==) | 4.83% | [118 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [src/parser/mod.rs](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?src=pr&el=tree&filepath=src%2Fparser%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL3BhcnNlci9tb2QucnM=) | 98.10% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion-sqlparser-rs/pull/2592?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] The great `Box`-ification experiment [datafusion-sqlparser-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2592:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2592#issuecomment-5847133047
<!-- dialect-labeler -->
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org