luoyuxia commented on code in PR #101:
URL: https://github.com/apache/paimon-rust/pull/101#discussion_r2878063451
##########
crates/paimon/src/io/file_io.rs:
##########
@@ -461,6 +457,55 @@ mod file_action_test {
file_io.delete_file(dst).await.unwrap();
}
+ async fn common_test_list_status_paths(file_io: &FileIO, dir_path: &str) {
+ if let Some(local_dir) = dir_path.strip_prefix("file:/") {
+ let _ = fs::remove_dir_all(local_dir);
+ }
+
+ file_io.mkdirs(dir_path).await.unwrap();
+
+ let file_a = format!("{dir_path}a.txt");
+ let file_b = format!("{dir_path}b.txt");
+ file_io
+ .new_output(&file_a)
+ .unwrap()
+ .write(Bytes::from("a"))
+ .await
+ .unwrap();
+ file_io
+ .new_output(&file_b)
+ .unwrap()
+ .write(Bytes::from("b"))
+ .await
+ .unwrap();
+
+ let statuses = file_io.list_status(dir_path).await.unwrap();
+ assert_eq!(statuses.len(), 2);
+
+ let expected_paths: BTreeSet<String> =
+ [file_a.clone(), file_b.clone()].into_iter().collect();
+ let actual_paths: BTreeSet<String> =
+ statuses.iter().map(|status| status.path.clone()).collect();
+ assert_eq!(
+ actual_paths, expected_paths,
+ "list_status should return exact entry paths"
+ );
+
+ assert!(
Review Comment:
Regarding to this test, Do we really need these following test code:
```
statuses.iter().all(|status| !status.is_dir),
"listed entries should be files in this test"
);
let sizes_by_path: BTreeMap<String, u64> = statuses
.iter()
.map(|status| (status.path.clone(), status.size))
.collect();
assert_eq!(sizes_by_path.get(&file_a), Some(&1));
assert_eq!(sizes_by_path.get(&file_b), Some(&1));
```
Can it be removed?
##########
crates/paimon/src/io/file_io.rs:
##########
@@ -137,7 +138,7 @@ impl FileIO {
statuses.push(FileStatus {
size: meta.content_length(),
is_dir: meta.is_dir(),
- path: entry.path().to_string(),
+ path: format!("{base_path}{}", entry.path()),
Review Comment:
Thanks. It sounds reasonable to me to return the full path with scheme
prefix.
But it doesn't seem to be necessary now for reusing the storage operator.
Maybe you could create a separate PR for reusing the storage operator part in
which we can revist it further it.
##########
crates/paimon/src/io/file_io.rs:
##########
@@ -461,6 +457,55 @@ mod file_action_test {
file_io.delete_file(dst).await.unwrap();
}
+ async fn common_test_list_status_paths(file_io: &FileIO, dir_path: &str) {
+ if let Some(local_dir) = dir_path.strip_prefix("file:/") {
+ let _ = fs::remove_dir_all(local_dir);
+ }
+
+ file_io.mkdirs(dir_path).await.unwrap();
+
+ let file_a = format!("{dir_path}a.txt");
+ let file_b = format!("{dir_path}b.txt");
+ file_io
Review Comment:
nit:
can be simplify to
```
for file in [&file_a, &file_b] {
file_io
.new_output(&file)
.unwrap()
.write(Bytes::from("test data"))
.await
.unwrap();
}
```
?
--
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]
