Uh oh!
There was an error while loading. Please reload this page.
Add a mutable visitor - #782
Conversation
This adds the ability to mutate parsed sql queries. Previously, only visitors taking an immutable reference to the visited structures were allowed.
Pull Request Test Coverage Report for Build 3818810193
💛 - Coveralls |
alamb
left a comment
There was a problem hiding this comment.
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)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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. |
alamb
commented
Jan 2, 2023
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.
Thank you 3ac3c67 looks good |
alamb
commented
Jan 2, 2023
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
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
commented
Jan 2, 2023
Yes I will go make one now |
alamb
commented
Jan 2, 2023
Preparing one now: #786 |
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.