westonpace commented on code in PR #12625:
URL: https://github.com/apache/arrow/pull/12625#discussion_r864990614
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -654,7 +655,8 @@ TEST(Substrait, ReadRel) {
ASSERT_EQ(scan_node_options.dataset->type_name(), "filesystem");
const auto& dataset =
checked_cast<const
dataset::FileSystemDataset&>(*scan_node_options.dataset);
- EXPECT_THAT(dataset.files(), ElementsAre("/tmp/dat1.parquet",
"/tmp/dat2.parquet"));
+ EXPECT_THAT(dataset.files(),
+ UnorderedElementsAre("/tmp/dat1.parquet", "/tmp/dat2.parquet"));
EXPECT_EQ(dataset.format()->type_name(), "parquet");
EXPECT_EQ(*dataset.schema(), Schema({field("i", int64()), field("b",
boolean())}));
}
Review Comment:
I agree that the majority of tests should go into the filesystems utilities
tests. It would be nice if we could have at least one test in the serde logic
just to make sure we are handling the case where a glob URI is specified (e.g.
coverage for that specific switch case branch, parsing the URI to extract the
glob pattern) but I recognize that gets more complicated.
It might make more sense to add these tests in when we add non-local
filesystem support to Substrait. I just created ARROW-16463. Can you add a
note to that JIRA about making sure we test the different types of file options?
--
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]
