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

consider exposing pre-built engines public #153

Closed
BugenZhao opened this issue Jan 17, 2023 · 6 comments · Fixed by #154
Closed

consider exposing pre-built engines public #153

BugenZhao opened this issue Jan 17, 2023 · 6 comments · Fixed by #154

Comments

@BugenZhao
Copy link
Collaborator

Hi, thanks for your awesome work! I'm a developer from RisingWave Labs 😄, and we're recently developing our customized simulation tester with the sqllogictest library.

RisingWave is a database that is mostly compatible with the PostgreSQL wire protocol. To implement a runner for the simulated RisingWave, we have to copy the code from bin/source/engines/postgres.rs and slightly wrap it with some other logic. This can actually be achieved by forwarding the implementation to the Postgres as AsyncDb, so I'm considering whether it's possible to expose this pre-built engine so that we can have better synchronization with the upstream?

If you don't mind, I can submit a PR to work on it. 🥰

@skyzh
Copy link
Member

skyzh commented Jan 17, 2023

Sure, feel free to send a PR to do that. I'm also thinking if we need to have different crates for each engine. e.g., sqllogictest-postgres, sqllogictest-postgres-extended, so that it can be better leveraged by other projects.

@BugenZhao
Copy link
Collaborator Author

Good advice! I'll first try to extract them into a new crate named sqllogictest-engine. Maybe we can make each specific engine optional based on cargo features, in the future.

@xxchan
Copy link
Member

xxchan commented Jan 17, 2023

It makes total sense, and actually arrow-datafusion has similar needs apache/datafusion#4834 (comment) cc @alamb

@xxchan
Copy link
Member

xxchan commented Jan 17, 2023

Just came up with a little quesion: how should the new crate be versioned? 🤔 Will there be any compatibility issues for combination of versions etc?

related, although different: #152

@alamb
Copy link
Contributor

alamb commented Jan 17, 2023

Just came up with a little quesion: how should the new crate be versioned? 🤔 Will there be any compatibility issues for combination of versions etc?

The way we handle this in arrow-rs / parquet is not to try and test with various different versions, but always increment the different subcrates in lockstep even if it isn't strictly necessary to do so and still conform to semver. Basically we didn't feel we had the time to do the proper testing of different versions

So that would mean everytime you released a sqllogic version you would release an upgrade of the underlying engine as well

@BugenZhao
Copy link
Collaborator Author

Will there be any compatibility issues for combination of versions etc?

I guess so. I've tried to implement my own sqllogictest::AsyncDB by forwarding the implementation to sqllogictest_engine::postgres::Postgres and failed to call the run method. This seems to be caused by inconsistent versions between the direct dependency of sqllogictest and indirect one of that from sqllogictest_engine. After I switch to the same version (repo), it works well.

So maybe we can decouple the version of bin(CLI) with other libraries, but should bump the versions of these libraries simultaneously.

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

Successfully merging a pull request may close this issue.

4 participants