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 2022/09/15 08:07:20 UTC
[GitHub] [arrow] vibhatha opened a new pull request, #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
vibhatha opened a new pull request, #14132:
URL: https://github.com/apache/arrow/pull/14132
This PR is work in progress.
--
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
[GitHub] [arrow] vibhatha commented on a diff in pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #14132:
URL: https://github.com/apache/arrow/pull/14132#discussion_r971665499
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -3078,5 +3089,94 @@ TEST(Substrait, AggregateRelEmit) {
buf, {}, conversion_options);
}
+TEST(Substrait, ReadRelWithGlobFiles) {
+ compute::ExecContext exec_context;
+ arrow::dataset::internal::Initialize();
+
+ auto dummy_schema =
+ schema({field("A", int32()), field("B", int32()), field("C", int32())});
+
+ // creating a dummy dataset using a dummy table
+ auto table_1 = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20]
+ ])"});
+ auto table_2 = TableFromJSON(dummy_schema, {R"([
+ [11, 11, 110],
+ [13, 14, 120]
+ ])"});
+ auto table_3 = TableFromJSON(dummy_schema, {R"([
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+ auto expected_table = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20],
+ [11, 11, 110],
+ [13, 14, 120],
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+
+ std::vector<std::shared_ptr<Table>> input_tables = {table_1, table_2, table_3};
+ auto format = std::make_shared<arrow::dataset::IpcFileFormat>();
+ auto filesystem = std::make_shared<fs::LocalFileSystem>();
+ const std::vector<std::string> file_names = {"serde_test_1.arrow", "serde_test_2.arrow",
+ "serde_test_3.arrow"};
+
+ const std::string path_prefix = "substrait-globfiles-";
+ std::string glob_like_path;
+ int idx = 0;
+
+ // creating a vector to avoid out-of-scoping Temporary directory
+ // if out-of-scoped the written folder get wiped out
+ std::vector<std::unique_ptr<arrow::internal::TemporaryDir>> tempdirs;
+ for (size_t i = 0; i < file_names.size(); i++) {
+ ASSERT_OK_AND_ASSIGN(auto tempdir, arrow::internal::TemporaryDir::Make(path_prefix));
+ tempdirs.push_back(std::move(tempdir));
+ }
+ for (const auto& file_name : file_names) {
+ ASSERT_OK_AND_ASSIGN(auto file_path, tempdirs[idx]->path().Join(file_name));
+ std::string file_path_str = file_path.ToString();
+ WriteIpcData(file_path_str, filesystem, input_tables[idx++]);
+ std::string base_path = file_path_str.substr(0, file_path_str.find(path_prefix));
+ glob_like_path = "file://" + base_path + path_prefix + "*/serde_test_*.arrow";
+ }
+
+ ASSERT_OK_AND_ASSIGN(auto buf, internal::SubstraitFromJSON("Plan", R"({
+ "relations": [{
+ "rel": {
+ "read": {
+ "base_schema": {
+ "names": ["A", "B", "C"],
+ "struct": {
+ "types": [{
+ "i32": {}
+ }, {
+ "i32": {}
+ }, {
+ "i32": {}
+ }]
+ }
+ },
+ "local_files": {
+ "items": [
+ {
+ "uri_path_glob": ")" + glob_like_path +
+ R"(",
Review Comment:
I formatted the code, but this is what it produces.
--
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
[GitHub] [arrow] vibhatha commented on a diff in pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #14132:
URL: https://github.com/apache/arrow/pull/14132#discussion_r979137678
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -3078,5 +3089,97 @@ TEST(Substrait, AggregateRelEmit) {
buf, {}, conversion_options);
}
+TEST(Substrait, ReadRelWithGlobFiles) {
+#ifdef _WIN32
+ GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows";
+#endif
+ compute::ExecContext exec_context;
+ arrow::dataset::internal::Initialize();
+
+ auto dummy_schema =
+ schema({field("A", int32()), field("B", int32()), field("C", int32())});
+
+ // creating a dummy dataset using a dummy table
+ auto table_1 = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20]
+ ])"});
+ auto table_2 = TableFromJSON(dummy_schema, {R"([
+ [11, 11, 110],
+ [13, 14, 120]
+ ])"});
+ auto table_3 = TableFromJSON(dummy_schema, {R"([
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+ auto expected_table = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20],
+ [11, 11, 110],
+ [13, 14, 120],
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+
+ std::vector<std::shared_ptr<Table>> input_tables = {table_1, table_2, table_3};
+ auto format = std::make_shared<arrow::dataset::IpcFileFormat>();
+ auto filesystem = std::make_shared<fs::LocalFileSystem>();
+ const std::vector<std::string> file_names = {"serde_test_1.arrow", "serde_test_2.arrow",
+ "serde_test_3.arrow"};
+
+ const std::string path_prefix = "substrait-globfiles-";
+ std::string glob_like_path;
+ int idx = 0;
+
+ // creating a vector to avoid out-of-scoping Temporary directory
+ // if out-of-scoped the written folder get wiped out
+ std::vector<std::unique_ptr<arrow::internal::TemporaryDir>> tempdirs;
+ for (size_t i = 0; i < file_names.size(); i++) {
+ ASSERT_OK_AND_ASSIGN(auto tempdir, arrow::internal::TemporaryDir::Make(path_prefix));
+ tempdirs.push_back(std::move(tempdir));
+ }
+ for (const auto& file_name : file_names) {
Review Comment:
Ah thanks for catching this one.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14132:
URL: https://github.com/apache/arrow/pull/14132#issuecomment-1247739647
https://issues.apache.org/jira/browse/ARROW-17640
--
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
[GitHub] [arrow] vibhatha commented on pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #14132:
URL: https://github.com/apache/arrow/pull/14132#issuecomment-1256842029
@westonpace Updated the PR and the `appveyor` is failing because of something related `Gandiva`.
--
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
[GitHub] [arrow] ursabot commented on pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14132:
URL: https://github.com/apache/arrow/pull/14132#issuecomment-1299480587
Benchmark runs are scheduled for baseline = 474c7a1cb91298bcabe5bed7e0716c0b6ac7820c and contender = a1b161e85a4dce497d65325db945f72bbd20b729. a1b161e85a4dce497d65325db945f72bbd20b729 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/4940648ea1f240bd9159f34fa841c0de...9ef1bfd358b0455fa0a960e9408202d4/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/f033b8274a1e4132b9236e039d8c5184...f3482c72c8304041992ae5426fb27225/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d4bf1a5c09fd42db8edadae0bbd12a67...2169d49b357c48b98ffac9514a7c5492/)
[Finished :arrow_down:0.18% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/6102a10bd6a24b099fb1ae0ad910e97c...bf748b3d848d4645ad99041ba80f7ccc/)
Buildkite builds:
[Finished] [`a1b161e8` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1803)
[Failed] [`a1b161e8` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1824)
[Finished] [`a1b161e8` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1791)
[Finished] [`a1b161e8` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1816)
[Finished] [`474c7a1c` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1802)
[Failed] [`474c7a1c` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1823)
[Finished] [`474c7a1c` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1790)
[Finished] [`474c7a1c` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1815)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] vibhatha commented on pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on PR #14132:
URL: https://github.com/apache/arrow/pull/14132#issuecomment-1249965879
cc @ArianaVillegas @sanjibansg
--
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
[GitHub] [arrow] westonpace commented on a diff in pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #14132:
URL: https://github.com/apache/arrow/pull/14132#discussion_r978942979
##########
cpp/src/arrow/filesystem/filesystem_test.cc:
##########
@@ -308,28 +308,27 @@ TEST(InternalUtil, GlobFiles) {
ASSERT_EQ(actual, expected);
};
- ASSERT_OK(fs->CreateDir("A/CD"));
- ASSERT_OK(fs->CreateDir("AB/CD"));
- ASSERT_OK(fs->CreateDir("AB/CD/ab"));
- CreateFile(fs.get(), "A/CD/ab.txt", "data");
- CreateFile(fs.get(), "AB/CD/a.txt", "data");
- CreateFile(fs.get(), "AB/CD/abc.txt", "data");
- CreateFile(fs.get(), "AB/CD/ab/c.txt", "data");
+ ASSERT_OK(fs->CreateDir(base_dir + "A/CD"));
+ ASSERT_OK(fs->CreateDir(base_dir + "AB/CD"));
+ ASSERT_OK(fs->CreateDir(base_dir + "AB/CD/ab"));
+ CreateFile(fs.get(), base_dir + "A/CD/ab.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/a.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/abc.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/ab/c.txt", "data");
FileInfoVector infos;
- ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, "A*/CD/?b*.txt"));
+ ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, base_dir + "A*/CD/?b*.txt"));
ASSERT_EQ(infos.size(), 2);
- check_entries(infos, {"A/CD/ab.txt", "AB/CD/abc.txt"});
+ check_entries(infos, {base_dir + "A/CD/ab.txt", base_dir + "AB/CD/abc.txt"});
- // Leading slash is optional but doesn't change behavior
- ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, "/A*/CD/?b*.txt"));
- ASSERT_EQ(infos.size(), 2);
- check_entries(infos, {"A/CD/ab.txt", "AB/CD/abc.txt"});
Review Comment:
Yes, that seems fine to me.
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -3078,5 +3089,97 @@ TEST(Substrait, AggregateRelEmit) {
buf, {}, conversion_options);
}
+TEST(Substrait, ReadRelWithGlobFiles) {
+#ifdef _WIN32
+ GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows";
+#endif
+ compute::ExecContext exec_context;
+ arrow::dataset::internal::Initialize();
+
+ auto dummy_schema =
+ schema({field("A", int32()), field("B", int32()), field("C", int32())});
+
+ // creating a dummy dataset using a dummy table
+ auto table_1 = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20]
+ ])"});
+ auto table_2 = TableFromJSON(dummy_schema, {R"([
+ [11, 11, 110],
+ [13, 14, 120]
+ ])"});
+ auto table_3 = TableFromJSON(dummy_schema, {R"([
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+ auto expected_table = TableFromJSON(dummy_schema, {R"([
+ [1, 1, 10],
+ [3, 4, 20],
+ [11, 11, 110],
+ [13, 14, 120],
+ [21, 21, 210],
+ [23, 24, 220]
+ ])"});
+
+ std::vector<std::shared_ptr<Table>> input_tables = {table_1, table_2, table_3};
+ auto format = std::make_shared<arrow::dataset::IpcFileFormat>();
+ auto filesystem = std::make_shared<fs::LocalFileSystem>();
+ const std::vector<std::string> file_names = {"serde_test_1.arrow", "serde_test_2.arrow",
+ "serde_test_3.arrow"};
+
+ const std::string path_prefix = "substrait-globfiles-";
+ std::string glob_like_path;
+ int idx = 0;
+
+ // creating a vector to avoid out-of-scoping Temporary directory
+ // if out-of-scoped the written folder get wiped out
+ std::vector<std::unique_ptr<arrow::internal::TemporaryDir>> tempdirs;
+ for (size_t i = 0; i < file_names.size(); i++) {
+ ASSERT_OK_AND_ASSIGN(auto tempdir, arrow::internal::TemporaryDir::Make(path_prefix));
+ tempdirs.push_back(std::move(tempdir));
+ }
+ for (const auto& file_name : file_names) {
Review Comment:
```suggestion
}
std::string sample_tempdir_path = tempdirs[0]->path();
std::string base_tempdir_path = sample_tempdir_path.substr(0, sample_tempdir_path.find(path_prefix));
std::string glob_like_path = "file://" + base_tempdir_path + path_prefix + "*/serde_test_*.arrow";
for (const auto& file_name : file_names) {
```
Nit: what you have works but I think it might be a little readable / clearer to calculate `glob_like_path` outside of the for loop.
##########
cpp/src/arrow/filesystem/path_util.h:
##########
@@ -75,6 +75,9 @@ util::string_view RemoveTrailingSlash(util::string_view s);
ARROW_EXPORT
Status AssertNoTrailingSlash(util::string_view s);
+ARROW_EXPORT
+Status AssertLeadingSlash(util::string_view s);
Review Comment:
Yes, it should probably return a bool and be named `HasLeadingSlash` but, at that point, the method is just...
```
bool HasLeadingSlash(util::string_view key) {
return key.front() == '/';
}
```
...and I'm not really sure such a method would be really needed.
--
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
[GitHub] [arrow] vibhatha commented on a diff in pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #14132:
URL: https://github.com/apache/arrow/pull/14132#discussion_r972506135
##########
cpp/src/arrow/filesystem/path_util.h:
##########
@@ -75,6 +75,9 @@ util::string_view RemoveTrailingSlash(util::string_view s);
ARROW_EXPORT
Status AssertNoTrailingSlash(util::string_view s);
+ARROW_EXPORT
+Status AssertLeadingSlash(util::string_view s);
Review Comment:
Should this be just return bool? It is a bit doubtful and tried to maintain the convention already in use.
cc @westonpace
--
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
[GitHub] [arrow] vibhatha commented on a diff in pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
vibhatha commented on code in PR #14132:
URL: https://github.com/apache/arrow/pull/14132#discussion_r972505830
##########
cpp/src/arrow/filesystem/filesystem_test.cc:
##########
@@ -308,28 +308,27 @@ TEST(InternalUtil, GlobFiles) {
ASSERT_EQ(actual, expected);
};
- ASSERT_OK(fs->CreateDir("A/CD"));
- ASSERT_OK(fs->CreateDir("AB/CD"));
- ASSERT_OK(fs->CreateDir("AB/CD/ab"));
- CreateFile(fs.get(), "A/CD/ab.txt", "data");
- CreateFile(fs.get(), "AB/CD/a.txt", "data");
- CreateFile(fs.get(), "AB/CD/abc.txt", "data");
- CreateFile(fs.get(), "AB/CD/ab/c.txt", "data");
+ ASSERT_OK(fs->CreateDir(base_dir + "A/CD"));
+ ASSERT_OK(fs->CreateDir(base_dir + "AB/CD"));
+ ASSERT_OK(fs->CreateDir(base_dir + "AB/CD/ab"));
+ CreateFile(fs.get(), base_dir + "A/CD/ab.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/a.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/abc.txt", "data");
+ CreateFile(fs.get(), base_dir + "AB/CD/ab/c.txt", "data");
FileInfoVector infos;
- ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, "A*/CD/?b*.txt"));
+ ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, base_dir + "A*/CD/?b*.txt"));
ASSERT_EQ(infos.size(), 2);
- check_entries(infos, {"A/CD/ab.txt", "AB/CD/abc.txt"});
+ check_entries(infos, {base_dir + "A/CD/ab.txt", base_dir + "AB/CD/abc.txt"});
- // Leading slash is optional but doesn't change behavior
- ASSERT_OK_AND_ASSIGN(infos, GlobFiles(fs, "/A*/CD/?b*.txt"));
- ASSERT_EQ(infos.size(), 2);
- check_entries(infos, {"A/CD/ab.txt", "AB/CD/abc.txt"});
Review Comment:
cc @westonpace
I removed this and updated the test case to function with and without leading `/`. Is this okay?
--
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
[GitHub] [arrow] westonpace merged pull request #14132: ARROW-17640: [C++] Add File Handling Test cases for GlobFile handling in Substrait Read
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #14132:
URL: https://github.com/apache/arrow/pull/14132
--
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