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

Always wrap statement in a named struct #1204

Open
tisonkun opened this issue Apr 7, 2024 · 5 comments
Open

Always wrap statement in a named struct #1204

tisonkun opened this issue Apr 7, 2024 · 5 comments

Comments

@tisonkun
Copy link
Contributor

tisonkun commented Apr 7, 2024

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
Copy link
Collaborator

alamb commented Apr 10, 2024

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
Copy link
Contributor Author

@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
Copy link
Collaborator

alamb commented Apr 11, 2024

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
Copy link
Contributor Author

@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
Copy link
Collaborator

alamb commented Apr 12, 2024

Sounds good -- thanks @tisonkun

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

No branches or pull requests

2 participants