fix: sqllogictest runner label condition mismatch #16633
Conversation
|
@gabotechs do you have time to review this PR? |
|
Thank you @lliangyu-lin 🙏 |
| setup_scratch_dir(&relative_path)?; | ||
|
|
||
| let count: u64 = get_record_count(&path, "DatafusionSubstraitRoundTrip".to_string()); | ||
| let count: u64 = get_record_count(&path, "DataFusion".to_string()); |
There was a problem hiding this comment.
We are going to need to keep the "DatafusionSubstraitRoundTrip" engine name for the future, we'll want to be able to ignore some statements based on it, something like:
skipif DatafusionSubstraitRoundTrip
Instead of changing the engine name, I'd go to the relevant tests in
And do the following replacement:
- onlyif DataFusion
+ skipif postgres
That way we maintain the ability to skip specific statements in DatafusionSubstraitRoundTrip mode, while prompting any engine other than postgres to actually try to comply with that test
There was a problem hiding this comment.
Thanks for the context! Updated to skipif postgres. In this case, do we still need to change the label Datafusion -> DataFusion? Since there are no more test files with condition onlyif DataFusion.
There was a problem hiding this comment.
I'd be nice to keep the label as DatafusionSubstraitRoundTrip so that we can use it in the future for skipping tests failing on Substrait roundtrip mode 👍
There was a problem hiding this comment.
Oh I guess my question was regarding the label DataFusion and Datafusion being used in the test files, which might be a typo, and not about DatafusionSubstraitRoundTrip.
But I saw Datafusion label is still being used in other test files, so I removed my changes.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @lliangyu-lin and @gabotechs
|
run extended tests |
3 participants
Which issue does this PR close?
Rationale for this change
The onlyif condition in sqllogictest test files are
DataFusionBut current runner attaches
DatafusionorDatafusionSubstraitRoundTripwhich doesn't exists in any of the test files. Hence, causing the statement / query to be skipped.What changes are included in this PR?
Are these changes tested?
Yes
cargo test --test sqllogictests -- --substrait-round-trip pg_compat_null.slt(passing)cargo test --test sqllogictests -- --substrait-round-trip pg_compat_union.slt(no longer seeing the table not found exeception, but [substrait] [sqllogictest] Unsupported producing row from empty relation #16271 still exists)cargo test --test sqllogictests -- --substrait-round-trip pg_compat_simple.slt(no longer seeing the table not found exeception)cargo test --test sqllogictests -- --substrait-round-trip pg_compat_window.slt(no longer seeing the table not found exeception)Are there any user-facing changes?
No