risinglightdb/sqllogictest-rs

bin: enhance `skipif`

xxchan opened this issue · 3 comments

xxchan commented

We have a skipif, added in #27, which uses DB::engine_name

/// Returns whether we should skip this record, according to given `conditions`.
fn should_skip(&self, conditions: &[Condition]) -> bool {
conditions
.iter()
.any(|c| c.should_skip(self.db.engine_name()))
}

#27 also added a flag --engine. But we use --engine to switch driver now, and the engine_name is not implemented in sqllogictest-bin

#[async_trait]
impl AsyncDB for Engines {
type Error = AnyhowError;
type ColumnType = DefaultColumnType;
async fn run(&mut self, sql: &str) -> Result<DBOutput<Self::ColumnType>, Self::Error> {
self.run(sql).await.map_err(AnyhowError)
}
}

I think adding a new flag like --name or --label can be enough.


Use case here risingwavelabs/risingwave#9013 (comment)

xxchan commented

I think the original use case is “SQL syntax varies between databases” (ref https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki), so the is just driver name. (But this is also break in out sqllogictest-bin now! 😄)

This new use case is that one driver’s different behavior under different configuration. Kind of like postgres+risingwave , postgres+risingwave-in-mem , postgres+cockroach

I'm thinking of whether we can decouple onlyif/skipif with engine name, and make it something like conditional compilation in Rust.

Specifically, users can add one or more lables by --cfg <label>, and use them as conditions by skipif <label1> <label2> .... The condition takes effect only if all labels are present. Another syntax may be introduced to express the OR logic.

In the use case above, there could be two labels: risingwave and in-mem. Users can choose any one or both of them to make condition. I think this design allows for more flexibility.

mamcx commented

Also, could be nice to skipif/onlyif for the whole file, like with control