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

Add a mutable visitor #782

Merged
merged 3 commits into from Jan 2, 2023
Merged

Add a mutable visitor #782

merged 3 commits into from Jan 2, 2023

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Dec 31, 2022

Happy new year 🥳

This is a follow-up on #765 .

This adds the ability to mutate the AST as it is visited. I didn't remove the old immutable-only visitor trait.
Previously, only visitors taking an immutable reference to the visited structures were allowed.

This adds the ability to mutate parsed sql queries.
Previously, only visitors taking an immutable reference to the visited structures were allowed.
@coveralls
Copy link

coveralls commented Dec 31, 2022

Pull Request Test Coverage Report for Build 3818810193

  • 0 of 28 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 85.875%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/visitor.rs 0 28 0.0%
Totals Coverage Status
Change from base Build 3801061789: -0.2%
Covered Lines: 13102
Relevant Lines: 15257

💛 - Coveralls

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thank you @lovasoa

Happy new year to you as well 🌟

cc @tustvold and @AugustoFKL

Prior to releasing this code, I think we need to bump the version of the derive crate (semantically compatible), and make sqlparser depend on that higher version (to get the new derive_mut visitor)

src/ast/visitor.rs Show resolved Hide resolved
src/ast/visitor.rs Show resolved Hide resolved
@lovasoa
Copy link
Contributor Author

lovasoa commented Jan 1, 2023

@alamb, I think taking the values by ownership would require a significant amount of work, and not bring performance gains.
For the version bumps, I'll make a new commit.

@alamb
Copy link
Collaborator

alamb commented Jan 2, 2023

@alamb, I think taking the values by ownership would require a significant amount of work, and not bring performance gains.

I don't think I did a good job explaining the alternative

The idea is to use something like https://github.com/apache/arrow-datafusion/blob/master/datafusion/expr/src/expr_rewriter.rs

However, I don't feel strongly about it -- the mutation approach looks good to me.

For the version bumps, I'll make a new commit.

Thank you 3ac3c67 looks good

@alamb alamb merged commit 524b8a7 into sqlparser-rs:main Jan 2, 2023
@alamb
Copy link
Collaborator

alamb commented Jan 2, 2023

I made a small toy version to compare the two, the by-value version is 4x slower even with a small struct:

BTW I think the more appropriate comparison is with release mode, but even then the example is 2x slower with ownership

https://play.rust-lang.org/?version=stable&mode=release&edition=2021

@lovasoa lovasoa deleted the visitor-mut branch January 2, 2023 13:21
@lovasoa
Copy link
Contributor Author

lovasoa commented Jan 2, 2023

Great, thanks for merging! I'll use that in sqlpage. Do you think you can release a new version on crates.io soon?

@alamb
Copy link
Collaborator

alamb commented Jan 2, 2023

Great, thanks for merging! I'll use that in sqlpage. Do you think you can release a new version on crates.io soon?

Yes I will go make one now

@alamb alamb mentioned this pull request Jan 2, 2023
@alamb
Copy link
Collaborator

alamb commented Jan 2, 2023

Preparing one now: #786

@alamb
Copy link
Collaborator

alamb commented Jan 2, 2023

https://crates.io/crates/sqlparser/0.30.0

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.

None yet

4 participants