risinglightdb / sqllogictest-rs

Sqllogictest (dialect with extensions) parser and runner in Rust.
Apache License 2.0
172 stars 45 forks source link

consider exposing pre-built `engines` public #153

Closed BugenZhao closed 1 year ago

BugenZhao commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

It makes total sense, and actually arrow-datafusion has similar needs https://github.com/apache/arrow-datafusion/pull/4834#discussion_r1064024091 cc @alamb

xxchan commented 1 year ago

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: https://github.com/risinglightdb/sqllogictest-rs/issues/152

alamb commented 1 year ago

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 commented 1 year ago

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.