Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid cloning during token creation #1603

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 14, 2024

Rationale

Inspired by @davisp's #1591 I was looking at Token::make_word and I noticed it made 2 clones (owned strings). Each copy slows things down so let's avoid them

Changes

  1. Change Token::make_word to reuse its argument and thus save a clone

API Changes

This changes Token::make_word and Token::make_keyword to take String instead of &str. This would be a breaking API change. If it is a problem I can make owned variants (like Token::make_word_owned or something) 🤔

Performance benchmarks:

++ critcmp main alamb_faster_keyword_lookup
group                                                    alamb_faster_keyword_lookup            main
-----                                                    ---------------------------            ----
sqlparser-rs parsing benchmark/sqlparser::select         1.00      6.4±0.03µs        ? ?/sec    1.00      6.4±0.03µs        ? ?/sec
sqlparser-rs parsing benchmark/sqlparser::with_select    1.02     31.5±0.10µs        ? ?/sec    1.00     31.0±0.13µs        ? ?/sec

🤔 it appears not to make much/any difference

@alamb
Copy link
Contributor Author

alamb commented Dec 14, 2024

I was looking at our benchmarks, and they are very limited. If we want to pursue optimizing sqlparser more I think it would probably be a good idea to add many queries (like TPCDS and TPCH) into the benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant