ARROW-2328: [C++] Fixed and unit tested feather writing with slice #1784

Adriandorr · 2018-03-23T12:04:43Z

When writing a slice of a table to feather, both the scalar data and the null bitmap were written out wrongly, i.e. from the beginning of the original table instead of from the offset.

scalar data is written from the offset
null bitmap is written from the offset.

I messed up my original pull request #1766, hence this replaces it. Hope that helps.

…f a table: - scalar data is written from the offset - null bitmap is written from the offset.

wesm · 2018-03-23T15:17:09Z

I will review this when I can, it has been a very busy week

…led because of the extra test added. Alos boolean didn't need to be treated special, all types data length should be calcutated as the rounded up to the nearest multiple of 8 of length*bit-width.

wesm · 2018-03-25T01:55:30Z

BTW, feel no need to put these "ARROW-2328: [C++]" messages in your commits. The only one that matters is in the PR title

Adriandorr · 2018-03-25T10:03:32Z

Will do, thanks.

pitrou · 2018-04-05T14:17:42Z

cpp/src/arrow/ipc/test-common.h

 }

-Status MakeStringTypesRecordBatch(std::shared_ptr<RecordBatch>* out) {
+Status MakeStringTypesRecordBatchWithNulls(std::shared_ptr<RecordBatch>* out,


Style note: if it's called MakeStringTypesRecordBatchWithNulls, it shouldn't take an argument indicating whether you want to include nulls. Consider renaming or using a private helper function.

Or simply add a bool with_nulls = true optional argument to MakeStringTypesRecordBatch.

Adding the default argument didn't work, because MakeStringTypesRecordBatch is used as a function pointer in other tests. I'll rename is to MakeStringTypesRecordBatchWithoutNulls abd this one to MakeStringTypesRecordBatch.

pitrou · 2018-04-05T14:19:17Z

cpp/src/arrow/ipc/feather-test.cc

+TEST_F(TestTableWriter, SliceRoundTrip) {
+  std::shared_ptr<RecordBatch> batch;
+  ASSERT_OK(MakeIntBatchSized(300, &batch));
+  batch = batch->Slice(100, 100);


It would be nice if you could iterate over a couple of different slice starts / lengths, especially non-multiples of 8.

pitrou · 2018-04-05T14:20:51Z

cpp/src/arrow/ipc/feather-test.cc

+  ASSERT_OK(MakeStringTypesRecordBatchWithNulls(&batch, false));
+  batch = batch->Slice(320, 30);
+
+  ASSERT_OK(writer_->Append("f0", *batch->column(0)));


There's a lot of duplicate code here with SliceRoundTrip.

pitrou · 2018-04-05T14:21:07Z

cpp/src/arrow/ipc/feather-test.cc

+
+  std::shared_ptr<Column> col;
+  ASSERT_OK(reader_->GetColumn(0, &col));
+  SCOPED_TRACE(col->data()->chunk(0)->ToString() + "\n" + batch->column(0)->ToString());


Is this required?

Yes(ish) It helps give a helpfull message when the test fails.

pitrou · 2018-04-05T14:22:00Z

cpp/src/arrow/ipc/feather-test.cc

+  ASSERT_EQ("f1", col->name());
+}
+
+TEST_F(TestTableWriter, SliceAtNonEightOffsetStringsWithNullsRoundTrip) {


There's no need writing independent methods here, just iterate over different slice starts / lengths.

I think the problem with loops in unit tests is that you only see the first failure. That said I should be able to convert these tests to one parameterised test, which hopefully also answers the above concerns..

Cool :-) Though I don't think using a loop is a big deal either...

pitrou · 2018-04-05T14:23:57Z

cpp/src/arrow/ipc/feather.cc

+                                    int64_t bit_offset, const int64_t length,
+                                    int64_t* bytes_written) {
+  data = data + bit_offset / 8;
+  uint8_t bit_shift = static_cast<uint8_t>(bit_offset % 8);


Is the cast to int8 necessary here? (for performance perhaps?)

Not really, but I think I got some warning, maybe with clang.

pitrou · 2018-04-05T14:29:00Z

cpp/src/arrow/ipc/feather.cc

@@ -558,15 +595,18 @@ class TableWriter::TableWriterImpl : public ArrayVisitor {
      // byte boundary, and we write this much data into the stream


This comment needs updating.

pitrou

Thanks for doing this. I added a couple review comments, please read.

…Batch.

Adriandorr · 2018-04-06T17:56:44Z

Thanks for the review comments. I’ve rewritten the tests as parameter unit tests and did most of the rest.

pitrou · 2018-04-07T09:57:19Z

cpp/src/arrow/ipc/feather-test.cc

+
+INSTANTIATE_TEST_CASE_P(
+    TestTableWriterSliceOffsets, TestTableWriterSlice,
+    ::testing::Values(std::make_tuple(300, 30), std::make_tuple(301, 30),


Hmm... Does gtest allow making a Cartesian product here?

Yes it does, yesterday I was just too lazy to find out how.

pitrou · 2018-04-07T09:58:45Z

cpp/src/arrow/ipc/feather-test.cc

+  ASSERT_OK(MakeIntBatchSized(start * 2, &batch));
+  batch = batch->Slice(start, size);
+
+  ASSERT_OK(writer_->Append("f0", *batch->column(0)));


This part and below is still duplicated in the other two test cases. Do you think you can factor that out, for example as a template function?

You are right the duplication can be removed without adding any complexity.

pitrou · 2018-04-07T10:00:24Z

cpp/src/arrow/ipc/test-common.h

  return Status::OK();
 }

+Status MakeStringTypesRecordBatchWithoutNulls(std::shared_ptr<RecordBatch>* out) {


Why is it called "...WithoutNulls" if it passes with_nulls = true?

Oops.. I'll rename it.

…ets and sizes.

pitrou · 2018-04-09T08:12:44Z

cpp/src/arrow/ipc/test-common.h

    } else {
-      const std::string& value = values[values_index];
+      const std::string value =
+          i < int64_t(values.size()) ? values[values_index] : std::to_string(i);


Why did you need this change? Is this just a debugging leftover?

The original random strings were not very random, just a repetition of the same few strings. I admit the new ones aren't very random either, in particular there are no repetitions which you would expect in a in real data. I probably should have added a new method (MakeRangeBinaryArray?).
Of course you don't want true random data either in a test.
If I revert the change I wouldn't be confident that my tests are still testing anything, the repetition might just align in a lucky pattern.
You ok with leaving it, I think it is better than the previous code?

I'd rather revert this change. If we want better random generation of binary arrays we should do it more thoroughly.

Not knowing the history of this particular function, I don't know what would be "better" in there. For my test I pretty much just want the consecutive numbers, otherwise it is very difficult to see what has gone wrong (I like the tests to give me the answer to that question if possible). I made a change to add a second function that implements that, but turns out this function is only used from MakeStringTypesRecordBatch, so we can't really have two implementations.

…rayWithUniqueValues and used that in MakeStringTypesRecordBatch.

pitrou · 2018-04-09T16:10:55Z

Thank you! I will merge once the AppVeyor build passes (the Travis-CI failures in the Rust and glib builds are unrelated).

(ARROW-2328: [C++] Fixed and unit tested feather writing with slice o…

4eb073c

…f a table: - scalar data is written from the offset - null bitmap is written from the offset.

Adriandorr mentioned this pull request Mar 23, 2018

ARROW-2328: [C++] Writing a slice with feather ignores the offset. #1766

Closed

Adriandorr added 2 commits March 23, 2018 13:36

(ARROW-2328: [C++] Fixed warning/error.

4bdb3b2

ARROW-2328: [C++] Fixed more warning/error.

7bd8170

wesm changed the title ~~(ARROW-2328: [C++] Fixed and unit tested feather writing with slice~~ ARROW-2328: [C++] Fixed and unit tested feather writing with slice Mar 23, 2018

Adriandorr added 2 commits March 24, 2018 11:54

{AD} Removed unused statement.

7ada731

ARROW-2328: [C++] On further investigation, exporting of booleans fai…

716650a

…led because of the extra test added. Alos boolean didn't need to be treated special, all types data length should be calcutated as the rounded up to the nearest multiple of 8 of length*bit-width.

pitrou reviewed Apr 5, 2018

View reviewed changes

pitrou requested changes Apr 5, 2018

View reviewed changes

Adriandorr added 4 commits April 6, 2018 17:44

Renamed MakeStringTypesRecordBatchWithNulls and MakeStringTypesRecord…

c259143

…Batch.

Updated comment.

de02b60

Converted test to parameterized tests.

98254d6

Fixed formating

d38fb6d

Fixed format.

80d9eed

pitrou reviewed Apr 7, 2018

View reviewed changes

Adriandorr added 3 commits April 7, 2018 11:36

Use commom CheckSlice method.

31e3521

Renamed MakeStringTypesRecordBatchWithoutNulls to its correct name.

03db69b

Changed the feather slice tests to run over cartesian product of offs…

7e18256

…ets and sizes.

pitrou reviewed Apr 9, 2018

View reviewed changes

{AD} Reverted changes in MakeRandomBinaryArray and added MakeBinaryAr…

a861113

…rayWithUniqueValues and used that in MakeStringTypesRecordBatch.

pitrou approved these changes Apr 9, 2018

View reviewed changes

pitrou closed this in 27417b2 Apr 9, 2018

		@@ -558,15 +595,18 @@ class TableWriter::TableWriterImpl : public ArrayVisitor {
		// byte boundary, and we write this much data into the stream

ARROW-2328: [C++] Fixed and unit tested feather writing with slice #1784

ARROW-2328: [C++] Fixed and unit tested feather writing with slice #1784

Uh oh!

Conversation

Adriandorr commented Mar 23, 2018

Uh oh!

wesm commented Mar 23, 2018

Uh oh!

wesm commented Mar 25, 2018

Uh oh!

Adriandorr commented Mar 25, 2018

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!

pitrou Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Adriandorr commented Apr 6, 2018

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!

pitrou commented Apr 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pitrou Apr 5, 2018 •

edited

Loading