Always wrap statement in a named struct

[tisonkun]

In GreptimeDB, we write code like:

#[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)]
pub struct Delete {
 // Can only be sqlparser::ast::Statement::Delete variant
 pub inner: Statement,
}

It's because in sqlparser-rs, these statement variants are unnamed struct:

 Delete {
 /// Multi tables delete are supported in mysql
 tables: Vec<ObjectName>,
 /// FROM
 from: Vec<TableWithJoins>,
 /// USING (Snowflake, Postgres, MySQL)
 using: Option<Vec<TableWithJoins>>,
 /// WHERE
 selection: Option<Expr>,
 /// RETURNING
 returning: Option<Vec<SelectItem>>,
 },

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

[alamb]

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

[tisonkun]

@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:

• Consensus: It would be nice to wrap statements in a named struct, in general.
• Options: Since the main concern is code change size and breaking changes for this crate and downstream, then:
  1. Do it at once, as previous efforts tried. Depend on the maintenance status, few ownership can drive such a decision (OT: this is a shared challenge for the Rust teams also).
  2. Do it "gradually". I'd admit "gradually" or actually continuously breaking downstream doesn't sound good. I mean, we can wrap one or several statements in named structs at a time (release), so each version bump don't surprise downstream users largely. And we can finally converged to every statement is in a named struct. I ever drived a few code migration like this in one to two years, e.g. FLINK-10392, TIDB-26022, but not for a library yet.

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.

[alamb]

Thank you @tisonkun for the nice summary

Do it "gradually". I'd admit "gradually" or actually continuously breaking downstream

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 Query and Statement 🤔 It seems like many of the sub enums might be ok to leave without their own dedicated struct

[tisonkun]

@alamb Cool.

Query is already wrapped in Query(Box<Query>). In our use case, wrapping Insert and Delete into a named struct can help.

I'd prepare a patch to update for these two variants in this month if there is no further concern.

[alamb]

Sounds good -- thanks @tisonkun