Repository navigation
Use identifiable structs for AST variant fields #311
Description
When consuming the generated AST for further processing, one needs to match against and deconstruct ast::Statement instances. Currently, the fields of this enum's variants are defined inline... which makes it impossible for downstream code to represent the nested structures as types. For example, consider this structure:
I propose we migrate to the following pattern.
pub struct AnalyzeStatement {
table_name: ObjectName,
partitions: Option<Vec<Expr>>,
for_columns: bool,
....
}
pub enum Statement {
Analyze(AnalyzeStatement),
...
}Obviously this is a massive breaking change, but that seems OK given the 0.9.x crates.io version.
Thoughts?
Activity
alamb commentedon Aug 20, 2021
Is the primary usecase of this proposal so that downstream code can re-use
AnalyzeStatementdirectly?cmoog commentedon Aug 20, 2021
Yes, exactly.
tvallotton commentedon Sep 2, 2021
I totally agree with this proposal. This would simplify my code considerably. I've got structs that are complete translations of some enum variants.
tvallotton commentedon Sep 4, 2021
I forked the project and made those changes if you want to see how it would look.
alamb commentedon Sep 9, 2021
I don't feel qualified to approve / accept this level of change without some additional buy in from @nickolay / @andygrove / @Dandandan . #311 (comment) seems like a good usecase to me but it is a pretty major change for all users of sqlparser-rs.
nickolay commentedon Dec 12, 2021
#40 could be marked as a duplicate of this issue as well. As mentioned there, I didn't think creating a separate struct for every single variant (even those holding one or two fields) of every enum was worth the breakage, but extracting complex variants like
Statement::Analyzeis something we have done before.In #315 (comment) @Dandandan suggested doing this for all variants of the
Statementenum, which seems a sensible suggestion too, as most statements exceed the complexity threshold anyway.alamb commentedon Dec 12, 2021
Your guidance makes a lot of sense to me @nickolay -- thank you
alamb commentedon Feb 8, 2022
Sadly I don't have the time to help drive projects such as this. Unless one of the other maintainers wants to do so, PRs like this are not likely to be merged. I am sorry I don't have the time and I am sorry if your time was wasted.
As the current state of affairs might not have been clear in the project's README. I have added #416 to try and help