You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "xitep (via GitHub)" <gi...@apache.org> on 2026/03/06 18:38:39 UTC
[PR] Expr:Function as a Box [datafusion-sqlparser-rs]
xitep opened a new pull request, #2271:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2271
* The whole change is just making `Expr::Function(Function)` a `Expr::Function(Box<Function>)` thereby reducing the size of `Expr` from 328 down to 216 bytes.
* This appears to be slightly improving the performance of the parser:
<details>
```
# on branch: `box-function`
> cargo bench -- --baseline main
...
sqlparser-rs parsing benchmark/sqlparser::select
time: [2.7318 µs 2.7401 µs 2.7489 µs]
change: [−8.0290% −7.4339% −6.8525%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
sqlparser-rs parsing benchmark/sqlparser::with_select
time: [14.875 µs 14.930 µs 14.995 µs]
change: [−5.0708% −4.3247% −3.5655%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
sqlparser-rs parsing benchmark/parse_large_statement
time: [4.7099 ms 4.7206 ms 4.7313 ms]
change: [−9.8526% −9.3668% −8.8572%] (p = 0.00 < 0.05)
Performance has improved.
sqlparser-rs parsing benchmark/format_large_statement
time: [268.01 µs 268.67 µs 269.37 µs]
change: [−1.2684% −0.8555% −0.3824%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe
word_to_ident/clone_into_ident_100x
time: [1.8360 µs 1.8463 µs 1.8572 µs]
change: [+2.2347% +2.8474% +3.5024%] (p = 0.00 < 0.05)
Performance has regressed.
word_to_ident/to_ident_100x
time: [1.5207 µs 1.5263 µs 1.5319 µs]
change: [+2.5991% +3.0039% +3.4031%] (p = 0.00 < 0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
parse_identifiers/select_100_columns
time: [71.498 µs 72.063 µs 72.674 µs]
change: [−6.9631% −6.2297% −5.5577%] (p = 0.00 < 0.05)
Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
6 (6.00%) high mild
6 (6.00%) high severe
parse_identifiers/select_100_qualified_columns
time: [151.78 µs 152.03 µs 152.30 µs]
change: [−5.3267% −4.6015% −3.4939%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe
```
</details>
* I was a bit surprised, to see this change actually improve a client program (which itself i cannot share, unfortunately):
<details>
```
# ./sqlfmt.sqlparser-main ... built against sqlparser's `main` branch (#64f4b1f)
# ./sqlfmt.sqlparser-function-box ... built ... well against the `function-box` branch proposed with this PR (#f8e9e18)
> hyperfine -N './sqlfmt.sqlparser-main /tmp/a.sql' './sqlfmt.sqlparser-function-box /tmp/a.sql'
Benchmark 1: ./sqlfmt.sqlparser-main /tmp/a.sql
Time (mean ± σ): 373.3 ms ± 2.1 ms [User: 278.6 ms, System: 94.4 ms]
Range (min … max): 370.2 ms … 377.9 ms 10 runs
Benchmark 2: ./sqlfmt.sqlparser-function-box /tmp/a.sql
Time (mean ± σ): 345.7 ms ± 1.5 ms [User: 254.3 ms, System: 91.2 ms]
Range (min … max): 343.2 ms … 348.4 ms 10 runs
Summary
./sqlfmt.sqlparser-function-box /tmp/a.sql ran
1.08 ± 0.01 times faster than ./sqlfmt.sqlparser-main /tmp/a.sql
> hyperfine -N './sqlfmt.sqlparser-main --optimize-column-aliases /tmp/a.sql' './sqlfmt.sqlparser-function-box --optimize-column-aliases /tmp/a.sql'
Benchmark 1: ./sqlfmt.sqlparser-main --optimize-column-aliases /tmp/a.sql
Time (mean ± σ): 411.9 ms ± 7.6 ms [User: 311.3 ms, System: 100.3 ms]
Range (min … max): 405.7 ms … 430.7 ms 10 runs
Benchmark 2: ./sqlfmt.sqlparser-function-box --optimize-column-aliases /tmp/a.sql
Time (mean ± σ): 381.2 ms ± 2.8 ms [User: 292.0 ms, System: 88.9 ms]
Range (min … max): 377.8 ms … 386.4 ms 10 runs
Summary
./sqlfmt.sqlparser-function-box --optimize-column-aliases /tmp/a.sql ran
1.08 ± 0.02 times faster than ./sqlfmt.sqlparser-main --optimize-column-aliases /tmp/a.sql
# the input file (/tmp/a.sql) is some ~50k lines of sql (approx 3.1mb) with some monster select
# from oracle's apex repeated approx 100 times
# the "--optimize-column-aliases" causes the program to traverse the AST a several times
# and allocate quite a lot of string, in other words it's inefficient, but that's the only different.
# Notably the absolute speed up is - more or less - the same for both versions, indicating
# a real win through the boxing in sqlparser's `Expr::Function`.
```
</details>
* My theory here is that by reducing the size of the `enum` CPU caches get better utilised manifesting in faster performance (at least when reading the AST.)
* `Expr` itself, could further _practically_ be reduced to about 80-100 bytes, though i'm not able to tell how much improvement that could contribute. (i've marked some candidates with `XXX` in this pr.)
* Other `enum`s/`struct`s, chiefly `Statement`, could be considered for this kind of change. there are really huge differences in sizes of the individual enum variants, leading `Parser::parse` (and related) to return needlessly large memory chunks (on average, of course.) `Expr` seemed very much prevalent in the AST, so i suppose this would be the biggest win / lowest hanging fruit.
* The biggest issue with this change are tests, though; we cannot pattern match on a `Box` is Rust, making it a pain in the neck to (re)write them without some additional helper functions (in this PR i just went straight to make it work without a lot of thought.)
* I would be glad if somebody could confirm or refute my theory. or maybe just run this change on some other machines. maybe @iffyio, @LucaCappelletti94, or @eyalleshem if you'd be interested?
* I'm keeping this as a draft for now, since my primary intention is first to trigger a discussion on how the API could (or not) evolve.
--
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] Expr::Function as a Box [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep commented on PR #2271:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2271#issuecomment-4013852360
absolutely no rush, @LucaCappelletti94. (this was just an experiment which surprised myself and i thought it's worth sharing.) if you'd manage to run it on some workload of yours it would be a great way to expose or exclude some error on my part.
--
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] Expr::Function as a Box [datafusion-sqlparser-rs]
Posted by "LucaCappelletti94 (via GitHub)" <gi...@apache.org>.
LucaCappelletti94 commented on PR #2271:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2271#issuecomment-4013387689
If there isn't rush, I can come back with benchmarks in parsing with and without this change against my fuzzing corpus. I am a bit overworked right now, so I don't think I can deliver it earlier than the end of next week.
--
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] Reduce size of `Expr` [datafusion-sqlparser-rs]
Posted by "xitep (via GitHub)" <gi...@apache.org>.
xitep closed pull request #2271: Reduce size of `Expr`
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2271
--
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