ARROW-3550: [C++] use kUnknownNullCount for the default null_count argument #3781
Conversation
cpp/src/arrow/array.h
Outdated
| @@ -325,7 +325,7 @@ class ARROW_EXPORT Array { | |||
| } else { | |||
| null_bitmap_data_ = NULLPTR; | |||
| } | |||
| data_ = data; | |||
| data_ = data->Copy(); | |||
There was a problem hiding this comment.
Any reason to explicit it?
There was a problem hiding this comment.
Copy() produces a deep copy, whereas data_ = data would mean that subsequently modifying the argument also modifies the array:
shared_ptr<ArrayData> somedata = ArrayData::Make(int32(), 0);
Int32Array arr(somedata);
somedata->length = 3;
arr->length(); // 3
There was a problem hiding this comment.
I had forgotten that data_ is a pointer and not the ArrayData object directly.1
There was a problem hiding this comment.
Are You sure that it is the expected behaviour? If so We should test it.
cc @pitrou
cpp/src/arrow/array.h
Outdated
| @@ -325,7 +325,7 @@ class ARROW_EXPORT Array { | |||
| } else { | |||
| null_bitmap_data_ = NULLPTR; | |||
| } | |||
| data_ = data; | |||
| data_ = data->Copy(); | |||
cpp/src/arrow/array.h
Outdated
| data->null_count = data->length; | ||
| data_ = data; | ||
| data_ = data->Copy(); | ||
| data_->null_count = data->length; |
There was a problem hiding this comment.
I do not think copying is correct. ArrayData is an internal API; copying should be addressed at a higher level
bkietz
force-pushed
the
ARROW-3550-array-constructor-kUnknownNullCount
branch
from
e18a1cd
to
fbe4915
Compare
|
|
||
| std::unique_ptr<Int32Array> arr_default_null_count( | ||
| new Int32Array(100, data, null_bitmap)); | ||
| ASSERT_EQ(kUnknownNullCount, arr_default_null_count->data()->null_count); |
There was a problem hiding this comment.
@wesm Is that sufficient or would you like to see similar cases for each affected array type?
|
will wait for build to run |
Previous default for this parameter in Array constructors was 0, which is not correct for all bitmaps the array might've been constructed with.
Sidecar: corrected
TypeTraits<NullType>to reflect that it is parameter free and has a type_singleton.