Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-44541: [C++] NumericArray<T> should not use ctor from parent directly #44542

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

GH-44541: [C++] NumericArray<T> should not use ctor from parent directly #44542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1896d63
GH-44541: NumericArray<T> should not use child's ctor directly
mapleFU Oct 28, 2024
cf54b48
fix comment and lint
mapleFU Oct 28, 2024
e83a3ed
Update: add Timestamp test, and fix lint
mapleFU Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions cpp/src/arrow/array/array_primitive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ class NumericArray : public PrimitiveArray {
using value_type = typename TypeClass::c_type;
using IteratorType = stl::ArrayIterator<NumericArray<TYPE>>;

explicit NumericArray(const std::shared_ptr<ArrayData>& data) { SetData(data); }
explicit NumericArray(const std::shared_ptr<ArrayData>& data) {
NumericArray::SetData(data);
}

// Only enable this constructor without a type argument for types without additional
// metadata
Expand All @@ -99,8 +101,16 @@ class NumericArray : public PrimitiveArray {
const std::shared_ptr<Buffer>& data,
const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
int64_t null_count = kUnknownNullCount, int64_t offset = 0) {
SetData(ArrayData::Make(TypeTraits<T1>::type_singleton(), length, {null_bitmap, data},
null_count, offset));
NumericArray::SetData(ArrayData::Make(TypeTraits<T1>::type_singleton(), length,
{null_bitmap, data}, null_count, offset));
}

NumericArray(std::shared_ptr<DataType> type, int64_t length,
const std::shared_ptr<Buffer>& data,
const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
int64_t null_count = kUnknownNullCount, int64_t offset = 0) {
NumericArray::SetData(ArrayData::Make(std::move(type), length, {null_bitmap, data},
null_count, offset));
}

const value_type* raw_values() const { return values_; }
Expand All @@ -119,7 +129,7 @@ class NumericArray : public PrimitiveArray {
IteratorType end() const { return IteratorType(*this, length()); }

protected:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of protected is odd here; nothing inherits from NumericArray inside arrow. Is it intended to support user subclassing of NumericArray? Or could we just mark NumericArray final? @pitrou

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might keep same style from PrimitiveArray, which initializes raw_values_ in ctor. Don't know should we change here

using PrimitiveArray::PrimitiveArray;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add NumericArray() if this line is deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 🤔

NumericArray() : values_(NULLPTR) {}

void SetData(const std::shared_ptr<ArrayData>& data) {
this->PrimitiveArray::SetData(data);
Expand Down
25 changes: 25 additions & 0 deletions cpp/src/arrow/array/array_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,31 @@ TEST_F(TestArray, TestValidateNullCount) {
}
}

TEST_F(TestArray, TestValidValues) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test case for TimestampArray specifically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// GH-44541: The value_ should be valid when construct.
{
std::vector<int32_t> original_data{1, 2, 3, 4, 5, 6, 7};
std::shared_ptr<Int32Array> arr =
std::make_shared<Int32Array>(::arrow::int32(), 7, Buffer::Wrap(original_data));
for (size_t i = 0; i < original_data.size(); ++i) {
EXPECT_TRUE(arr->IsValid(i));
EXPECT_FALSE(arr->IsNull(i));
EXPECT_EQ(original_data[i], arr->Value(i));
}
}
{
// Test non parameter free type.
std::vector<int64_t> original_data{1, 2, 3, 4, 5, 6, 7};
std::shared_ptr<TimestampArray> arr = std::make_shared<TimestampArray>(
::arrow::timestamp(TimeUnit::MICRO), 7, Buffer::Wrap(original_data));
for (size_t i = 0; i < original_data.size(); ++i) {
EXPECT_TRUE(arr->IsValid(i));
EXPECT_FALSE(arr->IsNull(i));
EXPECT_EQ(original_data[i], arr->Value(i));
}
}
}

void AssertAppendScalar(MemoryPool* pool, const std::shared_ptr<Scalar>& scalar) {
std::unique_ptr<arrow::ArrayBuilder> builder;
auto null_scalar = MakeNullScalar(scalar->type);
Expand Down
Loading