You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/14 18:11:52 UTC
[GitHub] [arrow] bkietz opened a new pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
bkietz opened a new pull request #8187:
URL: https://github.com/apache/arrow/pull/8187
Previously only the parent directories of files to be copied to a different filesystem were created.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494464587
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Another problem is that you're iterating on `dirs` while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#issuecomment-698590587
Non-Apache Travis: https://travis-ci.org/github/bkietz/arrow/builds/730077798
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494451602
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
no, `dirs.erase(ancestor, descendants_end)` would erase "a/b" too but I subtract 1 so it leaves the last descendant in place
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494324838
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(dirs.size()), [&](int i) {
+ return dirs[i].empty() ? Status::OK() : destination_fs->CreateDir(dirs[i]);
Review comment:
This will call `CreateDir` separately for intermediate directories, even though `recursive = true` is passed. This seems a bit wasteful, though I'm not sure it matters much in practice (it could be annoying on remote filesystems such as S3).
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
Review comment:
Interesting, I didn't know this `std::move` override!
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Hmm... if I have `{"a", "a/b", "a/c")`, it looks like this will remove `{"a", "a/b"}`?
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
In this example, `descendants_end` would be `dirs.end()`, though?
Perhaps you can fix this by using `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)` instead of `internal::IsAncestorOf(*ancestor, *descendants_end)`.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Another problem is that you're iterating on `dirs` while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Yet another issue is if you have `{"a", "a/b/c"}`, `"a"` won't be removed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r489598925
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
`cp file.ext new/path` fails if `new/path` doesn't exist. `cp dir newdir` fails if `dir` is a directory. But `cp -r dir newdir` works and makes nested directories.
So maybe the idiomatic solution is either (1) add a "recursive" argument, or (2) have a version that takes a single fs+FileSelector and a single destination fs+path, and that does the `cp -r` case.
The use case I'm interested in supporting is "copy the contents of this S3 bucket locally" and similar things.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#issuecomment-698590587
Non-Apache Travis: https://travis-ci.org/github/bkietz/arrow/builds/730077798
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #8187:
URL: https://github.com/apache/arrow/pull/8187
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494324838
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(dirs.size()), [&](int i) {
+ return dirs[i].empty() ? Status::OK() : destination_fs->CreateDir(dirs[i]);
Review comment:
This will call `CreateDir` separately for intermediate directories, even though `recursive = true` is passed. This seems a bit wasteful, though I'm not sure it matters much in practice (it could be annoying on remote filesystems such as S3).
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
Review comment:
Interesting, I didn't know this `std::move` override!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494453811
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r489734818
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
I'll add an argument to make directory creation optional, then
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r489737696
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
I would simply remove directory creation. A separate function for recursive copy can be implemented if desired.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494463542
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
In this example, `descendants_end` would be `dirs.end()`, though?
Perhaps you can fix this by using `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)` instead of `internal::IsAncestorOf(*ancestor, *descendants_end)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r488129142
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -16,6 +16,7 @@
// under the License.
#include <sstream>
+#include <unordered_set>
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494388617
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(dirs.size()), [&](int i) {
+ return dirs[i].empty() ? Status::OK() : destination_fs->CreateDir(dirs[i]);
Review comment:
I'll remove any directories which have a descendant in the set, that should produce the minimal number of directories to create.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494537039
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c") so "a" should be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r488777111
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
Yes, this doesn't look terrific at all, especially on a remote filesystem. Why do you need to create target directories on the fly at all? That doesn't seem very useful, nor idiomatic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494537039
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c")` so "a" should be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #8187:
URL: https://github.com/apache/arrow/pull/8187
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r489506318
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
Well, I could just remove the directory creation. @nealrichardson ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#issuecomment-697926431
@pitrou CI failure seems unrelated, PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494465714
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Yet another issue is if you have `{"a", "a/b/c"}`, `"a"` won't be removed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494388617
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -457,18 +458,52 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
ARROW_ASSIGN_OR_RAISE(auto source,
sources[i].filesystem->OpenInputStream(sources[i].path));
- auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
- if (!dest_dir.empty()) {
- RETURN_NOT_OK(destinations[i].filesystem->CreateDir(dest_dir));
- }
-
ARROW_ASSIGN_OR_RAISE(
auto destination,
destinations[i].filesystem->OpenOutputStream(destinations[i].path));
return internal::CopyStream(source, destination, chunk_size);
});
}
+Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
+ const FileSelector& source_sel,
+ const std::shared_ptr<FileSystem>& destination_fs,
+ const std::string& destination_base_dir, int64_t chunk_size,
+ bool use_threads) {
+ ARROW_ASSIGN_OR_RAISE(auto source_infos, source_fs->GetFileInfo(source_sel));
+ std::vector<FileLocator> sources, destinations;
+
+ std::unordered_set<std::string> destination_dirs;
+ destination_dirs.insert(destination_base_dir);
+
+ for (const FileInfo& source_info : source_infos) {
+ auto relative = internal::RemoveAncestor(source_sel.base_dir, source_info.path());
+ if (!relative.has_value()) {
+ return Status::Invalid("GetFileInfo() yielded path '", source_info.path(),
+ "', which is outside base dir '", source_sel.base_dir, "'");
+ }
+
+ auto destination_path =
+ internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
+
+ if (source_info.IsDirectory()) {
+ destination_dirs.insert(destination_path);
+ } else if (source_info.IsFile()) {
+ sources.push_back({source_fs, source_info.path()});
+ destinations.push_back({destination_fs, destination_path});
+ }
+ }
+
+ std::vector<std::string> dirs(destination_dirs.size());
+ std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(dirs.size()), [&](int i) {
+ return dirs[i].empty() ? Status::OK() : destination_fs->CreateDir(dirs[i]);
Review comment:
I'll remove any directories which have a descendant in the set, that should produce the minimal number of directories to create.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
no, `dirs.erase(ancestor, descendants_end)` would erase "a/b" too but I subtract 1 so it leaves the last descendant in place
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c") so "a" should be removed.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c") so "a" should be removed.
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c")` so "a" should be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494537039
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
> Perhaps `PathForest` can help. Though it's not obvious how to list the leaves.
I don't think so; the intent of PathForest is to visit all ancestors efficiently so getting a minimal set of descendants would not be efficient
> `internal::IsAncestorOf(*(descendants_end - 1), *descendants_end)`
You're right, thanks
> Another problem is that you're iterating on dirs while erasing from it. That's not safe, I think, unless you write `ancestor = dirs.erase(ancestor, descendants_end - 1)`.
You're right; vector::erase invalidates iterators *at* or after the beginning of the range.
> Yet another issue is if you have {"a", "a/b/c"}, "a" won't be removed?
No, I'd expect `IsAncestorOf("a", "a/b/c") so "a" should be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r494450952
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -487,15 +489,27 @@ Status CopyFiles(const std::shared_ptr<FileSystem>& source_fs,
internal::ConcatAbstractPath(destination_base_dir, relative->to_string());
if (source_info.IsDirectory()) {
- destination_dirs.insert(destination_path);
+ dirs.push_back(destination_path);
} else if (source_info.IsFile()) {
sources.push_back({source_fs, source_info.path()});
destinations.push_back({destination_fs, destination_path});
}
}
- std::vector<std::string> dirs(destination_dirs.size());
- std::move(destination_dirs.begin(), destination_dirs.end(), dirs.begin());
+ // remove directories with descendants since recursive directory creation will create
+ // them automatically
+ std::sort(dirs.begin(), dirs.end());
+ for (auto ancestor = dirs.begin(); ancestor != dirs.end(); ++ancestor) {
+ auto descendants_end = ancestor + 1;
+
+ while (descendants_end != dirs.end() &&
+ internal::IsAncestorOf(*ancestor, *descendants_end)) {
+ ++descendants_end;
+ }
+
+ dirs.erase(ancestor, descendants_end - 1);
Review comment:
Hmm... if I have `{"a", "a/b", "a/c")`, it looks like this will remove `{"a", "a/b"}`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#issuecomment-692231642
https://issues.apache.org/jira/browse/ARROW-10003
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8187: ARROW-10003: [C++] Create parent dir for any destination fs in CopyFiles
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8187:
URL: https://github.com/apache/arrow/pull/8187#discussion_r488129044
##########
File path: cpp/src/arrow/filesystem/filesystem.cc
##########
@@ -448,6 +449,13 @@ Status CopyFiles(const std::vector<FileLocator>& sources,
destinations.size(), " paths.");
}
+ RETURN_NOT_OK(::arrow::internal::OptionalParallelFor(
+ use_threads, static_cast<int>(sources.size()), [&](int i) {
+ auto dest_dir = internal::GetAbstractPathParent(destinations[i].path).first;
+ return dest_dir.empty() ? Status::OK()
+ : destinations[i].filesystem->CreateDir(dest_dir);
Review comment:
This may be very redundant, for example when copying lots of files into a single destination directory. If we had FileLocator::ToUri or similar we could maintain an unordered_set to ensure each directory is created only once
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org