[GitHub] [arrow-datafusion] melgenek commented on a diff in pull request #5033: [sqllogictest] Read subdirectories in `test_files`

["melgenek (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org