melgenek commented on code in PR #5033:
URL: https://github.com/apache/arrow-datafusion/pull/5033#discussion_r1084604332
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -110,13 +104,38 @@ async fn run_complete_file(
Ok(())
}
-fn read_test_files(options: &Options) -> Vec<PathBuf> {
- std::fs::read_dir(TEST_DIRECTORY)
- .unwrap()
- .map(|path| path.unwrap().path())
- .filter(|path| options.check_test_file(path.as_path()))
- .filter(|path| options.check_pg_compat_file(path.as_path()))
- .collect()
+fn read_test_files<'a>(
+ options: &'a Options,
+) -> Box<dyn Iterator<Item = (PathBuf, String)> + 'a> {
+ Box::new(
+ read_dir_recursive(TEST_DIRECTORY)
+ .map(|path| {
+ (
+ path.clone(),
+ path.to_string_lossy()
+ .strip_prefix(TEST_DIRECTORY)
+ .unwrap()
+ .to_string(),
+ )
+ })
+ .filter(|(_, file_name)| options.check_test_file(file_name))
+ .filter(|(path, _)| options.check_pg_compat_file(path.as_path())),
Review 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.
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -110,13 +104,38 @@ async fn run_complete_file(
Ok(())
}
-fn read_test_files(options: &Options) -> Vec<PathBuf> {
- std::fs::read_dir(TEST_DIRECTORY)
- .unwrap()
- .map(|path| path.unwrap().path())
- .filter(|path| options.check_test_file(path.as_path()))
- .filter(|path| options.check_pg_compat_file(path.as_path()))
- .collect()
+fn read_test_files<'a>(
+ options: &'a Options,
+) -> Box<dyn Iterator<Item = (PathBuf, String)> + 'a> {
+ Box::new(
+ read_dir_recursive(TEST_DIRECTORY)
+ .map(|path| {
+ (
+ path.clone(),
+ path.to_string_lossy()
Review 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"
```
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -47,13 +47,7 @@ pub async fn main() -> Result<(), Box<dyn Error>> {
let options = Options::new();
- let files: Vec<_> = read_test_files(&options);
-
- info!("Running test files {:?}", files);
-
- for path in files {
- let file_name =
path.file_name().unwrap().to_str().unwrap().to_string();
-
+ for (path, file_name) in read_test_files(&options) {
Review Comment:
`file_name` is technically not a file name here, but
`relative_file_path`/`relative_file_path`/`relative_file_name`.
I haven't changed the name yet, but it could be clearer to explain somehow
that the string is relative to the root directory.
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -110,13 +104,38 @@ async fn run_complete_file(
Ok(())
}
-fn read_test_files(options: &Options) -> Vec<PathBuf> {
- std::fs::read_dir(TEST_DIRECTORY)
- .unwrap()
- .map(|path| path.unwrap().path())
- .filter(|path| options.check_test_file(path.as_path()))
- .filter(|path| options.check_pg_compat_file(path.as_path()))
- .collect()
+fn read_test_files<'a>(
+ options: &'a Options,
+) -> Box<dyn Iterator<Item = (PathBuf, String)> + 'a> {
+ Box::new(
+ read_dir_recursive(TEST_DIRECTORY)
+ .map(|path| {
+ (
+ path.clone(),
+ path.to_string_lossy()
+ .strip_prefix(TEST_DIRECTORY)
+ .unwrap()
+ .to_string(),
+ )
+ })
+ .filter(|(_, file_name)| options.check_test_file(file_name))
+ .filter(|(path, _)| options.check_pg_compat_file(path.as_path())),
+ )
+}
+
+fn read_dir_recursive<P: AsRef<Path>>(path: P) -> Box<dyn Iterator<Item =
PathBuf>> {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
