[sqllogictest] Read subdirectories in test_files
#5033
Conversation
github-actions
bot
added
core
| ) | ||
| } | ||
|
|
||
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> { |
There was a problem hiding this comment.
WalkDir crate seemed like an overkill because there is no need for link resolution and similar features, but only recursive traversing.
Of course, WalkDir can be thrown in here instead of the custom code.
There was a problem hiding this comment.
I agree walkdir is not needed
There was a problem hiding this comment.
I think you might be able to avoid the Box using something like
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> { | |
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> impl Iterator<Item = PathBuf> { |
There was a problem hiding this comment.
The thing is that the resolved type for the whole implementation inside this function is FlatMap. But inside flat_map there is also std::iter::once for the case when there is only one file. And then rust cannot resolve a concrete implementation based on these options.
It is probably possible to apply your suggestion, but my rust skills were not good enough.
There was a problem hiding this comment.
I think https://docs.rs/itertools/latest/itertools/enum.Either.html# is one way to handle this. However, I don't think it is important to change
| .map(|path| { | ||
| ( | ||
| path.clone(), | ||
| path.to_string_lossy() |
There was a problem hiding this comment.
This is done to output not only names but relative paths. For example, a query from the pg_compat dir would be printed this way.
[pg_compat/pg_compat_union.slt] Running query: "DROP TABLE aggregate_test_100_by_sql"
| ) | ||
| }) | ||
| .filter(|(_, file_name)| options.check_test_file(file_name)) | ||
| .filter(|(path, _)| options.check_pg_compat_file(path.as_path())), |
There was a problem hiding this comment.
Postgres file filter doesn't account for folder names, but it can. For example, instead of expecting a pg_compat prefix on the file name, the runner could check this prefix on a relative file path.
| fn schema_name(relative_path: &Path) -> &str { | ||
| relative_path | ||
| .file_name() | ||
| .unwrap() |
There was a problem hiding this comment.
in general I think it is nice to avoid the unwrap() which panic's but in this case it would happen if the filenames were non utf8 -- so I think it is ok here
There was a problem hiding this comment.
I updated the logic to only keep Ascii characters 93f4ffd. I had issues with having numbers in postgres schema, so Ascii chars should be the safest option.
| ) | ||
| } | ||
|
|
||
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> { |
There was a problem hiding this comment.
I agree walkdir is not needed
| ) | ||
| } | ||
|
|
||
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> { |
There was a problem hiding this comment.
I think you might be able to avoid the Box using something like
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item = PathBuf>> { | |
| fn read_dir_recursive<P: AsRef<Path>>(path: P) -> impl Iterator<Item = PathBuf> { |
|
FYI any "no space left on device" CI errors should be fixed by merging up from master to get #5047 |
|
Benchmark runs are scheduled for baseline = b5d90b6 and contender = 2aa1490. 2aa1490 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Which issue does this PR close?
Closes #4709.
Rationale for this change
Tests could be categorized into folders.
What changes are included in this PR?
Sqllogictest reads
test_filesdirectory recursively.Are these changes tested?
Github actions.
Are there any user-facing changes?
No.