-
Notifications
You must be signed in to change notification settings - Fork 558
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
Always wrap statement in a named struct #1204
Comments
Here is a similar discussion in the past: #311 I agree it would be nice to do It would be a large code change both for this crate and downstream |
@alamb Thanks for your information! I'm glad to see it's discussed and previous efforts exist, so I don't have to spend the same time exploring those options. I can see we have one consensus and two options now:
In GreptimeDB, we gets used to upgrade Arrow's/DataFusion's version and handle API changes so it may not a big deal (or even the recent Hyper/Axum/Tonic breaking changes (link)). Looking forward to further thoughts. |
Thank you @tisonkun for the nice summary
I believe this is the only feasible solution for a crate like sqlparser-rs that is already in wide use and has lots of users. Maybe there are a few structs that are most important we could start with. For example |
@alamb Cool.
I'd prepare a patch to update for these two variants in this month if there is no further concern. |
Sounds good -- thanks @tisonkun |
In GreptimeDB, we write code like:
It's because in sqlparser-rs, these statement variants are unnamed struct:
I wonder if we can always wrap statement in a named struct so that downstream software can reuse the AST and impls (like for Display) more smoothly.
I don't know if it's a breaking change or we're generally OK with this.
Ref - GreptimeTeam/greptimedb#3646
The text was updated successfully, but these errors were encountered: