Skip to content

Thoughts on maintainership #84

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

Closed
benesch opened this issue May 29, 2019 · 5 comments
Closed

Thoughts on maintainership #84

benesch opened this issue May 29, 2019 · 5 comments

Comments

@benesch
Copy link
Contributor

benesch commented May 29, 2019

Hi @andygrove! 👋

First and foremost: thanks for all the work on this SQL parser! (With a big shoutout to @nickolay.) It's saved us a lot of time at @MaterializeInc, where we're using the parser in a SQL streaming dataflow engine.

I just got permission to open source our fork (https://github.com/MaterializeInc/sqlparser), which has a number of fixes on top of master. I've opened PRs for the first round of patches, but there's already a bunch of patches I haven't yet PR'd, and there's a bunch more improvements that we have planned.

We'd like to upstream as many patches as possible, but I'm nervous about the amount by which our fork is diverging already. (For example, improving standards compliance for CREATE TABLE is a rather invasive refactor: MaterializeInc/sqlparser@5fa7fe6.) I'm more than happy to do the work of extracting and PRing the upstreamable bits from our fork, but the work is exponential in the number of open, interdependent PRs, and I've already accumulated quite a few.

Since you mentioned your time is limited these days, I was wondering if you'd be comfortable letting me and @nickolay merge PRs without your approval. Totally understand if you'd rather build up to that level of trust, but I wanted to offer to step up as a maintainer to reduce the burden on you. I'd be happy to limit myself to those PRs which fix uncontroversial deficiencies, too, leaving some of the larger questions around supporting multiple dialects to when you have more time.

@nickolay
Copy link
Contributor

I should note that @andygrove has already authorized me to merge PRs:

I'd like us to continue with PRs rather than just pushing to master, but I'm fine if you merge without waiting for my approval (I really only have time at weekends right now for reviews)

Given that most of the PRs accumulated so far merged cleanly, I felt we could wait a bit to give Andy a chance to comment. But I guess Andy's away at the moment, so I'll go ahead and merge the PRs I've already looked through (#80, #77, #76, #75, #73, #72).

Going forward, I'd prefer to work through the changes at my own pace, rather than being a bottleneck. The PRs from you all looked great, the tweaks I did suggest were fairly minor and could just as well have been applied in a follow-up. So I'm in favor of adding you as a maintainer (not that anyone asked:)

Please take a look at the older PRs - I was going to merge them as well, but now that I've glanced at the commits in your repo, I'm afraid to make the job of upstreaming your changes harder than it needs to be (my #65 in particular seems to overlap with your changes, and I'm happy to adapt as needed, as I don't have as large a patch queue as yours).

One other thing I wanted to mention is that I got sqlparser to be clean from clippy warnings in #51 (but didn't enable it on CI). Some of the clippy's recommendations were good and it wasn't too much work otherwise, so I recommend running it periodically (rls-vscode will show the warnings as you edit after you install clippy).

@benesch
Copy link
Contributor Author

benesch commented May 31, 2019

Awesome, thanks so much, @nickolay! I'm one patchset away from being able to parse all of the SQL logic tests (https://www.sqlite.org/sqllogictest) and all the TPC-H queries (https://github.com/electrum/tpch-dbgen/tree/master/queries) in our fork—as soon as I finish that up I'll start upstreaming things again.

@andygrove
Copy link
Member

Hi @benesch! Thanks for the contributions and thanks again to @nickolay for keeping momentum going on this project. I've taken on some challenges in my day job and I have zero time right now for open source work. I'm hoping that will change in the future but I'm very happy to see a community forming around this project and I don't want to slow things down. I have invited you to join as a collaborator.

There was talk of a Rust database working group and I was planning on offering to donate this project to that effort. I'll see if I can find out the status of that.

@benesch
Copy link
Contributor Author

benesch commented May 31, 2019

Thanks very much, @andygrove. I saw you posted about the status of the WG at https://internals.rust-lang.org/t/kickstarting-a-database-wg/9696/45; I'll keep an eye on that thread to see how things go.

@benesch
Copy link
Contributor Author

benesch commented Jul 6, 2019

Well, the database working group doesn't seem to be going anywhere fast, and the process we've got in place here seems to be working well in the meantime, so I'm going to close this out.

@benesch benesch closed this as completed Jul 6, 2019
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

3 participants