ARROW-12500: [C++][Datasets] Ensure better test coverage of Dataset file formats by lidavidm · Pull Request #10134 · apache/arrow

lidavidm · 2021-04-22T17:56:12Z

This unifies (most of) the tests across Parquet, Feather, and CSV (with carve-outs for particular cases). In particular, this means all formats are now tested in conjunction with async/sync and serial/threaded scanners. Also, a set of common file format tests were refactored out of the individual tests and centralized.

…mats

lidavidm · 2021-04-22T17:58:44Z

-
+  // Properly set conversion types even for non-materialized fields
+  // (since we're reading all of them anyways)
+  for (auto field : scan_options->dataset_schema->fields()) {


Unifying the tests led me to discover this, which I think inadvertently fixes a bug I ran into in ARROW-9697 that I hadn't figured out before. There, I found that for CSV fragments to_table(schema=..., columns=[]) would fail when to_table(schema=...) did not fail, because in the former case, the CSV reader would complain about an invalid cast, seemingly ignoring the schema set.

I think this isn't the best solution, however; we should actually be deriving include_columns.

github-actions · 2021-04-22T18:03:09Z

https://issues.apache.org/jira/browse/ARROW-12500

westonpace

This is great. Getting rid of a lot of duplicate code. I still want to review the tests removed but here are some initial thoughts.

westonpace · 2021-04-22T18:57:38Z

    if (column_names.find(field->name()) == column_names.end()) continue;
    convert_options.column_types[field->name()] = field->type();
+    // Only read the requested columns
+    if (materialized_fields.find(field->name()) == materialized_fields.end()) continue;


Pretty minor nit but maybe move this above convert_options.column_types... so that convert_options.column_types doesn't include columns that are not in convert_options.include_columns.

westonpace · 2021-04-22T19:01:47Z

 namespace dataset {

-class TestCsvFileFormat : public testing::TestWithParam<Compression::type> {
+class ArrowCsvWriterMixin {


Naming nit: Not sure mixin is the right term here. When I think mixin I think multiple inheritance. Maybe Strategy? Also, why "ArrowCsv". Are there other CSV writers? Also, maybe there is a way to use composition here? Have an abstract method in FileFormatFixtureMixin that returns a FormatStrategy or something? I know "prefer composition over inheritance" but I don't know if there is a similar "prefer composition over template" guideline 😆

I originally tried with abstract methods, but then because of the inheritance tree, you'd have to re-implement them in the AbcFileFormatTest and in the AbcFileFormatScanTest fixtures (I had issues when trying to just have the latter inherit the former). Probably there is some combination of superclasses and interfaces that'll make it work but at that point I decided a template was the lesser evil.

westonpace · 2021-04-22T19:05:27Z

+  void TestOpenFailureWithRelevantError(FileFormat* format, StatusCode code) {
+    std::shared_ptr<Buffer> buf = std::make_shared<Buffer>(util::string_view(""));
+    auto result = format->Inspect(FileSource(buf));
+    EXPECT_FALSE(result.ok());


These ok checks seem redundant given we are not testing Result/Status here and you test the code on the next line.

westonpace · 2021-04-22T19:08:03Z

+  }
+
+  // Shared test cases
+  void TestOpenFailureWithRelevantError(FileFormat* format, StatusCode code) {


Rename to TestInspectFailureWithRelevantError? There is no method named Open on FileFormat.

westonpace · 2021-04-22T19:09:55Z

@@ -199,18 +210,11 @@ TEST_P(TestCsvFileFormat, OpenFailureWithRelevantError) {
  if (GetCompression() != Compression::type::UNCOMPRESSED) {
    GTEST_SKIP() << "File source name is different with compression";


Is this really a problem? Compression is just adding to the filename and we are testing with HasSubstr.

westonpace · 2021-04-22T19:25:24Z

+    ASSERT_OK(SetProjection(opts_.get(), opts_->dataset_schema->field_names()));
+  }
+
+  void SetFilter(Expression filter) {


Not for this PR but I feel like there are a number of places where we have to do something like this to maintain implicit invariants. It might be nice to have a ScanOptions cleanup at some point. Change the name so it isn't Options, hide it from the public API, give it more methods (like SetFilter) and private state.

Added to ARROW-12311.

westonpace · 2021-04-22T19:38:33Z

+    ASSERT_OK_AND_ASSIGN(supported, format->IsSupported(*source));
+    EXPECT_EQ(supported, true);
+  }
+  std::shared_ptr<Buffer> TestWrite(FileFormat* format, std::shared_ptr<Schema> schema,


Nit: This is more helper function than test. Perhaps rename to WriteToBuffer? Also, we could create a TestWrite here that does...

auto reader = GetRecordBatchReader(schema({field("f64", float64())})); auto source = GetFileSource(reader.get()); auto written = WriteToBuffer(format_.get(), reader->schema()); AssertBufferEqual(*written, *source->buffer());

...that should be universally applicable.

westonpace · 2021-04-22T19:41:47Z

+    auto dataset = std::make_shared<FragmentDataset>(schema, FragmentVector{fragment});
+    ScannerBuilder builder(dataset, opts_);
+    ARROW_EXPECT_OK(builder.UseAsync(GetParam().use_async));
+    ARROW_EXPECT_OK(builder.UseThreads(GetParam().use_threads));


Probably no need to set UseThreads here, that will not affect how the FileFormat is interacted with.

westonpace · 2021-04-22T19:46:13Z

+    ASSERT_EQ(row_count, GetParam().expected_rows());
+  }
+
+  void TestScanProjected(FileFormat* format) {


If projection is applied by the scanner why are we testing it here?

This is testing whether each format will look at the projection in the ScanOptions and only return the necessary columns to fulfill the projection + filter later on. Notably, CSV didn't properly do that before.

westonpace · 2021-04-22T19:49:12Z

+  }
+
+  // Scan the fragment through the scanner.
+  RecordBatchIterator Batches(std::shared_ptr<Fragment> fragment) {


What does this add to FileFormat testing above and beyond ScanBatches? One could argue that PhysicalBatches is all you need.

Originally I wanted to have coverage of each file type with the actual scanner (right now we test each format in isolation + the scanner in isolation). But that is overkill as you point out.

I could be easily convinced either way. I think it's also good to detect potential interaction bugs. I suppose the scanner could do various things with scan options (like the projection issue you mentioned) that aren't so easily exhaustively tested in formats.

On balance I think I'll refactor this since you plan to add fuller integration tests for datasets which is a better spot to catch more involved things. The test cases here I'll keep, but only to directly test Scan/ScanBatches.

lidavidm · 2021-04-22T20:24:13Z

Ah, despite all the diffs, no tests should have been removed - only shuffled around or consolidated when they were the same exact test.

westonpace

Should have been clearer earlier. I just wanted to do a quick pass to make sure that no tests got missed in the refactor. But I can't see any so looks good. I don't think any of my change in this PR or the other are required.

westonpace · 2021-04-22T20:53:31Z

+    auto sink = CreateOutputStream(pool);
+
+    ARROW_EXPECT_OK(WriteRecordBatchReader(reader, pool, sink));
+    // XXX the rest of the test may crash if this fails, since out will be nullptr


Minor but could the method be changed to return Result so this comment can go away?

lidavidm · 2021-04-23T12:25:36Z

Thanks for the quick review. I think everything's been addressed.

lidavidm added 4 commits April 22, 2021 13:46

ARROW-12500: [C++][Dataset] Test scanner with IPC as well

e309e77

ARROW-12500: [C++][Dataset] Consolidate tests between IPC/Parquet for…

8c301f2

…mats

ARROW-12500: [C++][Dataset] Run Parquet tests for both sync/async modes

049820d

ARROW-12500: [C++][Dataset] Unify CSV tests with IPC/Parquet

545a543

lidavidm added the Component: C++ label Apr 22, 2021

lidavidm commented Apr 22, 2021

View reviewed changes

lidavidm added 2 commits April 22, 2021 14:12

ARROW-12500: [C++][Dataset] Only read necessary CSV columns

bef1355

ARROW-12500: [C++][Dataset] Fix clang warnings

4c594d9

westonpace reviewed Apr 22, 2021

View reviewed changes

westonpace approved these changes Apr 22, 2021

View reviewed changes

lidavidm added 2 commits April 22, 2021 17:18

ARROW-12500: [C++][Dataset] Address review comments

5e15bda

ARROW-12500: [C++][Dataset] Don't test full scanner for every format

3bb73ba

lidavidm closed this in b254a30 Apr 26, 2021

asfimport mentioned this pull request May 3, 2021

[C++][Dataset] Consolidate similar tests for file formats #28265

Closed

		@@ -199,18 +210,11 @@ TEST_P(TestCsvFileFormat, OpenFailureWithRelevantError) {
		if (GetCompression() != Compression::type::UNCOMPRESSED) {
		GTEST_SKIP() << "File source name is different with compression";

Conversation

lidavidm commented Apr 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 22, 2021

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Apr 22, 2021

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Apr 23, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants