Support filtering specific sqllogictests identified by line number #16029
Conversation
7169d21 to
9dbba35
Compare
ae5c3c4 to
51ab0eb
Compare
| file_substring: parts[1].to_string(), | ||
| line_number: Some(line), | ||
| }), | ||
| Err(_) => Err(format!("Cannot parse line number from '{s}'")), |
There was a problem hiding this comment.
should we use DataFusionError::External(...) ?
There was a problem hiding this comment.
For this one we should be fine with plain strings, as this is just used by clap for parsing whatever we provide in CLI arguments, so not really DataFusion related.
| // statement might be populating tables that future test cases will use. | ||
| if !matches!( | ||
| sql_stmt.as_ref(), | ||
| SqlStatement::Query(_) | SqlStatement::Explain { .. } |
There was a problem hiding this comment.
should we also consider set statements (SetVariable) per query like: set datafusion.sql_parser.dialect?
There was a problem hiding this comment.
By "consider" you mean "make them skippable"? As SET statements dictate the behavior of future test cases, I don't think we can skip them
| return false; | ||
| }; | ||
|
|
||
| if matches!(statement, Statement::CreateExternalTable(_)) { |
There was a problem hiding this comment.
I think we can move this logic to another function like statement_is_skippable
There was a problem hiding this comment.
nice suggestion! I factored it out and it looks much clearer now
There was a problem hiding this comment.
Thank you @gabotechs and @LiaCastaneda -- this looks very nice to me
I had one small comment which we can do as a follow on PR (or never).
I also wonder about testing the test harness, but I think that may be unecessary given how often these tools are run
My guess is that it's not too bad to not have tests in the new |
|
I agree -- we can add unit tests if there is an issue or we want to make more sophisticated behavior |
|
Thanks again @gabotechs |
Which issue does this PR close?
Rationale for this change
Being able to execute only certain sqllogictest cases identified by line number.
As an example, the following command
cargo test --test sqllogictests -- aggregate:6991will run the test in
datafusion/datafusion/sqllogictest/test_files/aggregate.slt
Line 6991 in e60b260
What changes are included in this PR?
Extend the filtering argument in the sqllogictest binary to support filtering tests identified by line number and file name substring
Are these changes tested?
N/A
Are there any user-facing changes?
Just extensions of the sqllogictest execution CLI for local development