Skip to content

ARROW-212: Change contract of PrimitiveArray to reflect its abstractness#87

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

Closed
emkornfield wants to merge 1 commit into from
Closed

ARROW-212: Change contract of PrimitiveArray to reflect its abstractness #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions cpp/src/arrow/types/primitive.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -162,6 +162,11 @@ BooleanArray::BooleanArray(int32_t length, const std::shared_ptr<Buffer>& data,
: PrimitiveArray(
std::make_shared<BooleanType>(), length, data, null_count, null_bitmap) {}

BooleanArray::BooleanArray(const TypePtr& type, int32_t length,
const std::shared_ptr<Buffer>& data, int32_t null_count,
const std::shared_ptr<Buffer>& null_bitmap)
: PrimitiveArray(type, length, data, null_count, null_bitmap) {}

bool BooleanArray::EqualsExact(const BooleanArray& other) const {
if (this == &other) return true;
if (null_count_ != other.null_count_) { return false; }
Expand Down
15 changes: 9 additions & 6 deletions cpp/src/arrow/types/primitive.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,11 +34,10 @@ namespace arrow {

class MemoryPool;

// Base class for fixed-size logical types
// Base class for fixed-size logical types. See MakePrimitiveArray
// (types/construct.h) for constructing a specific subclass.
class PrimitiveArray : public Array {
public:
PrimitiveArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& data,
int32_t null_count = 0, const std::shared_ptr<Buffer>& null_bitmap = nullptr);
virtual ~PrimitiveArray() {}

const std::shared_ptr<Buffer>& data() const { return data_; }
Expand All@@ -47,6 +46,8 @@ class PrimitiveArray : public Array {
bool Equals(const std::shared_ptr<Array>& arr) const override;

protected:
PrimitiveArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& data,
int32_t null_count = 0, const std::shared_ptr<Buffer>& null_bitmap = nullptr);
std::shared_ptr<Buffer> data_;
const uint8_t* raw_data_;
};
Expand All@@ -55,12 +56,14 @@ class PrimitiveArray : public Array {
class NAME : public PrimitiveArray { \
public: \
using value_type = T; \
using PrimitiveArray::PrimitiveArray; \
\
NAME(int32_t length, const std::shared_ptr<Buffer>& data, int32_t null_count = 0, \
const std::shared_ptr<Buffer>& null_bitmap = nullptr) \
: PrimitiveArray( \
std::make_shared<TypeClass>(), length, data, null_count, null_bitmap) {} \
NAME(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& data, \
int32_t null_count = 0, const std::shared_ptr<Buffer>& null_bitmap = nullptr) \
: PrimitiveArray(type, length, data, null_count, null_bitmap) {} \
\
bool EqualsExact(const NAME& other) const { \
return PrimitiveArray::EqualsExact(*static_cast<const PrimitiveArray*>(&other)); \
Expand DownExpand Up@@ -261,10 +264,10 @@ typedef NumericBuilder<DoubleType> DoubleBuilder;

class BooleanArray : public PrimitiveArray {
public:
using PrimitiveArray::PrimitiveArray;

BooleanArray(int32_t length, const std::shared_ptr<Buffer>& data,
int32_t null_count = 0, const std::shared_ptr<Buffer>& null_bitmap = nullptr);
BooleanArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& data,

@xhochyxhochyJun 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the benefit of passing a TypePtr? This array should always have the same type?

@emkornfieldemkornfieldJun 7, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In theory it could have a different logical type (I can't think of a great example for boolean types). I'm also including it here because we still haven't established if we want to be very pedantic about not doing operations that could fail with bad_alloc (especially in constructors).

Its a slightly bigger change but I can remove the requirement for passing this type and the the other ones on primitive. I don't have a strong preference here.

@xhochyxhochyJun 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense, forgot that logical types are part of the TypePtr

int32_t null_count = 0, const std::shared_ptr<Buffer>& null_bitmap = nullptr);

bool EqualsExact(const BooleanArray& other) const;
bool Equals(const ArrayPtr& arr) const override;
Expand Down