ARROW-1280: [C++] add fixed size list type#4278

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
bkietz wants to merge 8 commits into from
Closed

ARROW-1280: [C++] add fixed size list type #4278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9258dd
add fixed size list type
bkietz May 8, 2019
dae1275
removing unused offset buffer from constructor
bkietz May 9, 2019
59ce758
fixing some typos/copypasta
bkietz May 9, 2019
3256051
Add ArrayFromJSON impl for fixed size list
bkietz May 9, 2019
ddb46fd
Add pretty-print test for fixed size list
bkietz May 9, 2019
17b55c5
Fix underline in Layout.rst
bkietz May 9, 2019
ca02bda
add test and missing impl for ipc/json+fixed_size_list
bkietz May 9, 2019
920a251
correct ListArray equality comparison
bkietz May 10, 2019
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
214 changes: 214 additions & 0 deletions cpp/src/arrow/array-list-test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
auto type = list(int32());
auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
auto offset = 2;
EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
public:
void SetUp() {
TestBuilder::SetUp();

value_type_ = int32();
type_ = fixed_size_list(value_type_, list_size());

std::unique_ptr<ArrayBuilder> tmp;
ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
}

void Done() {
std::shared_ptr<Array> out;
FinishAndCheckPadding(builder_.get(), &out);
result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
}

protected:
static constexpr int32_t list_size() { return 2; }
std::shared_ptr<DataType> value_type_;

std::shared_ptr<FixedSizeListBuilder> builder_;
std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

std::shared_ptr<Array> array, equal_array, unequal_array;
std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

// setup two equal arrays
ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
ASSERT_OK(builder_->Finish(&array));

ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

ASSERT_OK(builder_->Finish(&equal_array));

// now an unequal one
ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
ASSERT_OK(builder_->Finish(&unequal_array));

// Test array equality
AssertArraysEqual(*array, *array);
AssertArraysEqual(*array, *equal_array);
EXPECT_FALSE(equal_array->Equals(unequal_array));
EXPECT_FALSE(unequal_array->Equals(equal_array));

// Test range equality
EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
ASSERT_OK(builder_->AppendNull());
ASSERT_OK(builder_->AppendNull());

Done();

ASSERT_OK(ValidateArray(*result_));
ASSERT_TRUE(result_->IsNull(0));
ASSERT_TRUE(result_->IsNull(1));

ASSERT_EQ(0, result_->value_offset(0));
ASSERT_EQ(list_size(), result_->value_offset(1));

auto values = result_->values();
ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
ASSERT_OK(builder_->AppendNulls(3));

Done();

ASSERT_OK(ValidateArray(*result_));
ASSERT_EQ(result_->length(), 3);
ASSERT_EQ(result_->null_count(), 3);
ASSERT_TRUE(result_->IsNull(0));
ASSERT_TRUE(result_->IsNull(1));
ASSERT_TRUE(result_->IsNull(2));

ASSERT_EQ(0, result_->value_offset(0));
ASSERT_EQ(list_size(), result_->value_offset(1));
ASSERT_EQ(list_size() * 2, result_->value_offset(2));

auto values = result_->values();
ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
const std::vector<int32_t>& values,
const std::vector<uint8_t>& is_valid) {
ASSERT_OK(ValidateArray(*result));
ASSERT_EQ(1, result->null_count());
ASSERT_LE(result->values()->null_count(), 2);

ASSERT_EQ(3, result->length());
for (int32_t i = 0; i < 3; ++i) {
ASSERT_EQ(i * result->value_length(), result->value_offset(i));
}

for (int i = 0; i < result->length(); ++i) {
ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
}

ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

for (size_t i = 0; i < values.size(); ++i) {
if (is_valid[i / result->value_length()] == 0) {
continue;
}
ASSERT_EQ(values[i], varr->Value(i));
}
}

TEST_F(TestFixedSizeListArray, TestBasics) {
std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
std::vector<uint8_t> is_valid = {1, 0, 1};

Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

int pos = 0;
for (size_t i = 0; i < values.size() / list_size(); ++i) {
if (is_valid[i] == 0) {
ASSERT_OK(builder_->AppendNull());
pos += list_size();
continue;
}
ASSERT_OK(builder_->Append());
for (int j = 0; j < list_size(); ++j) {
ASSERT_OK(vb->Append(values[pos++]));
}
}

Done();
ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
std::vector<uint8_t> is_valid = {1, 0, 1};

Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
for (int32_t value : values) {
ASSERT_OK(vb->Append(value));
}
Done();
ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
std::vector<uint8_t> is_valid = {1, 0, 1};

Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
for (int32_t value : values) {
ASSERT_OK(vb->Append(value));
}
for (int32_t value : values) {
ASSERT_OK(vb->Append(value));
}

Done();
ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
// All buffers are null
Done();
ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

std::unique_ptr<ArrayBuilder> tmp;
ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

ASSERT_OK(builder_->AppendValues(4));

std::shared_ptr<Array> list_array;
ASSERT_OK(builder_->Finish(&list_array));

@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.

@bkietzbkietzMay 9, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array


const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow
54 changes: 54 additions & 0 deletions cpp/src/arrow/array.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
int64_t length,
const std::shared_ptr<Array>& values,
const std::shared_ptr<Buffer>& null_bitmap,
int64_t null_count, int64_t offset) {
auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
internal_data->child_data.emplace_back(values->data());
SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
this->Array::SetData(data);

DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
list_size_ = list_type()->list_size();

DCHECK_EQ(data_->child_data.size(), 1);
values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
return ValidateOffsets(array);
}

Status Visit(const FixedSizeListArray& array) {
if (array.length() < 0) {
return Status::Invalid("Length was negative");
}
if (!array.values()) {
return Status::Invalid("values was null");
}
if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.
Outdated
return Status::Invalid(
"Values Length (", array.values()->length(), ") was not equal to the length (",
array.length(), ") multiplied by the list size (", array.value_length(), ")");
}

return Status::OK();
}

Status Visit(const StructArray& array) {
if (array.length() < 0) {
return Status::Invalid("Length was negative");
Expand Down
37 changes: 37 additions & 0 deletions cpp/src/arrow/array.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {

@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it worth having common abstract class?

@bkietzbkietzMay 9, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.

FixedSizeListScalar and ListScalar are identical but also trivial.

@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).

@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

public:
using TypeClass = FixedSizeListType;

explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
const std::shared_ptr<Array>& values,
const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
int64_t null_count = kUnknownNullCount, int64_t offset = 0);

const FixedSizeListType* list_type() const;

/// \brief Return array object containing the list's values
std::shared_ptr<Array> values() const;

std::shared_ptr<DataType> value_type() const;

// Neither of these functions will perform boundschecking
int32_t value_offset(int64_t i) const {
i += data_->offset;
return static_cast<int32_t>(list_size_ * i);
}
int32_t value_length(int64_t i = 0) const { return list_size_; }

protected:
void SetData(const std::shared_ptr<ArrayData>& data);
int32_t list_size_;

private:
std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-1280: [C++] add fixed size list type#4278

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

bkietz
wants to merge
8
commits into

from

 Closed


ARROW-1280: [C++] add fixed size list type
#4278
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
f9258dd

add fixed size list type


bkietz May 8, 2019
dae1275

removing unused offset buffer from constructor


bkietz May 9, 2019
59ce758

fixing some typos/copypasta


bkietz May 9, 2019
3256051

Add ArrayFromJSON impl for fixed size list


bkietz May 9, 2019
ddb46fd

Add pretty-print test for fixed size list


bkietz May 9, 2019
17b55c5

Fix underline in Layout.rst


bkietz May 9, 2019
ca02bda

add test and missing impl for ipc/json+fixed_size_list


bkietz May 9, 2019
920a251

correct ListArray equality comparison


bkietz May 10, 2019

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


214 changes: 214 additions & 0 deletions
cpp/src/arrow/array-list-test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -124,6 +124,14 @@ TEST_F(TestListArray, Equality) {
 ASSERT_TRUE(array->RangeEquals(1, 5, 0, slice));
}

TEST_F(TestListArray, ValuesEquality) {
 auto type = list(int32());
 auto left = ArrayFromJSON(type, "[[1, 2], [3], [0]]");
 auto right = ArrayFromJSON(type, "[[1, 2], [3], [100000]]");
 auto offset = 2;
 EXPECT_FALSE(left->Slice(offset)->Equals(right->Slice(offset)));
}

TEST_F(TestListArray, TestResize) {}

TEST_F(TestListArray, TestFromArrays) {
Expand DownExpand Up@@ -332,4 +340,210 @@ TEST_F(TestListArray, TestBuilderPreserveFieleName) {
 ASSERT_EQ("counts", type.value_field()->name());
}

// ----------------------------------------------------------------------
// FixedSizeList tests

class TestFixedSizeListArray : public TestBuilder {
 public:
 void SetUp() {
 TestBuilder::SetUp();

 value_type_ = int32();
 type_ = fixed_size_list(value_type_, list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, type_, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));
 }

 void Done() {
 std::shared_ptr<Array> out;
 FinishAndCheckPadding(builder_.get(), &out);
 result_ = std::dynamic_pointer_cast<FixedSizeListArray>(out);
 }

 protected:
 static constexpr int32_t list_size() { return 2; }
 std::shared_ptr<DataType> value_type_;

 std::shared_ptr<FixedSizeListBuilder> builder_;
 std::shared_ptr<FixedSizeListArray> result_;
};

TEST_F(TestFixedSizeListArray, Equality) {
 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 std::shared_ptr<Array> array, equal_array, unequal_array;
 std::vector<int32_t> equal_values = {1, 2, 3, 4, 5, 2, 2, 2, 5, 6};
 std::vector<int32_t> unequal_values = {1, 2, 2, 2, 3, 4, 5, 2};

 // setup two equal arrays
 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));
 ASSERT_OK(builder_->Finish(&array));

 ASSERT_OK(builder_->AppendValues(equal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(equal_values.data(), equal_values.size()));

 ASSERT_OK(builder_->Finish(&equal_array));

 // now an unequal one
 ASSERT_OK(builder_->AppendValues(unequal_values.size() / list_size()));
 ASSERT_OK(vb->AppendValues(unequal_values.data(), unequal_values.size()));
 ASSERT_OK(builder_->Finish(&unequal_array));

 // Test array equality
 AssertArraysEqual(*array, *array);
 AssertArraysEqual(*array, *equal_array);
 EXPECT_FALSE(equal_array->Equals(unequal_array));
 EXPECT_FALSE(unequal_array->Equals(equal_array));

 // Test range equality
 EXPECT_TRUE(array->RangeEquals(0, 1, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(0, 2, 0, unequal_array));
 EXPECT_FALSE(array->RangeEquals(1, 2, 1, unequal_array));
 EXPECT_TRUE(array->RangeEquals(1, 3, 2, unequal_array));
}

TEST_F(TestFixedSizeListArray, TestAppendNull) {
 ASSERT_OK(builder_->AppendNull());
 ASSERT_OK(builder_->AppendNull());

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 2, values->length());
}

TEST_F(TestFixedSizeListArray, TestAppendNulls) {
 ASSERT_OK(builder_->AppendNulls(3));

 Done();

 ASSERT_OK(ValidateArray(*result_));
 ASSERT_EQ(result_->length(), 3);
 ASSERT_EQ(result_->null_count(), 3);
 ASSERT_TRUE(result_->IsNull(0));
 ASSERT_TRUE(result_->IsNull(1));
 ASSERT_TRUE(result_->IsNull(2));

 ASSERT_EQ(0, result_->value_offset(0));
 ASSERT_EQ(list_size(), result_->value_offset(1));
 ASSERT_EQ(list_size() * 2, result_->value_offset(2));

 auto values = result_->values();
 ASSERT_EQ(list_size() * 3, values->length());
}

void ValidateBasicFixedSizeListArray(const FixedSizeListArray* result,
 const std::vector<int32_t>& values,
 const std::vector<uint8_t>& is_valid) {
 ASSERT_OK(ValidateArray(*result));
 ASSERT_EQ(1, result->null_count());
 ASSERT_LE(result->values()->null_count(), 2);

 ASSERT_EQ(3, result->length());
 for (int32_t i = 0; i < 3; ++i) {
 ASSERT_EQ(i * result->value_length(), result->value_offset(i));
 }

 for (int i = 0; i < result->length(); ++i) {
 ASSERT_EQ(is_valid[i] == 0, result->IsNull(i));
 }

 ASSERT_EQ(result->length() * result->value_length(), result->values()->length());
 auto varr = std::dynamic_pointer_cast<Int32Array>(result->values());

 for (size_t i = 0; i < values.size(); ++i) {
 if (is_valid[i / result->value_length()] == 0) {
 continue;
 }
 ASSERT_EQ(values[i], varr->Value(i));
 }
}

TEST_F(TestFixedSizeListArray, TestBasics) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 int pos = 0;
 for (size_t i = 0; i < values.size() / list_size(); ++i) {
 if (is_valid[i] == 0) {
 ASSERT_OK(builder_->AppendNull());
 pos += list_size();
 continue;
 }
 ASSERT_OK(builder_->Append());
 for (int j = 0; j < list_size(); ++j) {
 ASSERT_OK(vb->Append(values[pos++]));
 }
 }

 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppend) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 Done();
 ValidateBasicFixedSizeListArray(result_.get(), values, is_valid);
}

TEST_F(TestFixedSizeListArray, BulkAppendInvalid) {
 std::vector<int32_t> values = {0, 1, 2, 3, 4, 5};
 std::vector<uint8_t> is_valid = {1, 0, 1};

 Int32Builder* vb = checked_cast<Int32Builder*>(builder_->value_builder());

 ASSERT_OK(builder_->AppendValues(values.size() / list_size(), is_valid.data()));
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }
 for (int32_t value : values) {
 ASSERT_OK(vb->Append(value));
 }

 Done();
 ASSERT_RAISES(Invalid, ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestZeroLength) {
 // All buffers are null
 Done();
 ASSERT_OK(ValidateArray(*result_));
}

TEST_F(TestFixedSizeListArray, TestBuilderPreserveFieleName) {
 auto list_type_with_name = fixed_size_list(field("counts", int32()), list_size());

 std::unique_ptr<ArrayBuilder> tmp;
 ASSERT_OK(MakeBuilder(pool_, list_type_with_name, &tmp));
 builder_.reset(checked_cast<FixedSizeListBuilder*>(tmp.release()));

 ASSERT_OK(builder_->AppendValues(4));

 std::shared_ptr<Array> list_array;
 ASSERT_OK(builder_->Finish(&list_array));
@pitroupitrouMay 9, 2019

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.

IIUC, this builds an invalid FixedSizeListArray? No values are appended to the child array.
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It does build an invalid array, but the point of this test is just to check that the field name is correctly propagated to the built array

 const auto& type = checked_cast<FixedSizeListType&>(*list_array->type());
 ASSERT_EQ("counts", type.value_field()->name());
}

} // namespace arrow

54 changes: 54 additions & 0 deletions
cpp/src/arrow/array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -295,6 +295,44 @@ std::shared_ptr<DataType> ListArray::value_type() const {

std::shared_ptr<Array> ListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// FixedSizeListArray

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

FixedSizeListArray::FixedSizeListArray(const std::shared_ptr<DataType>& type,
 int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap,
 int64_t null_count, int64_t offset) {
 auto internal_data = ArrayData::Make(type, length, {null_bitmap}, null_count, offset);
 internal_data->child_data.emplace_back(values->data());
 SetData(internal_data);
}

void FixedSizeListArray::SetData(const std::shared_ptr<ArrayData>& data) {
 DCHECK_EQ(data->type->id(), Type::FIXED_SIZE_LIST);
 this->Array::SetData(data);

 DCHECK(list_type()->value_type()->Equals(data->child_data[0]->type));
 list_size_ = list_type()->list_size();

 DCHECK_EQ(data_->child_data.size(), 1);
 values_ = MakeArray(data_->child_data[0]);
}

const FixedSizeListType* FixedSizeListArray::list_type() const {
 return checked_cast<const FixedSizeListType*>(data_->type.get());
}

std::shared_ptr<DataType> FixedSizeListArray::value_type() const {
 return list_type()->value_type();
}

std::shared_ptr<Array> FixedSizeListArray::values() const { return values_; }

// ----------------------------------------------------------------------
// String and binary

Expand DownExpand Up@@ -839,6 +877,22 @@ struct ValidateVisitor {
 return ValidateOffsets(array);
 }

 Status Visit(const FixedSizeListArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
 }
 if (!array.values()) {
 return Status::Invalid("values was null");
 }
 if (array.values()->length() != array.length() * array.value_length()) {
Comment thread
bkietz marked this conversation as resolved.


Outdated

 return Status::Invalid(
 "Values Length (", array.values()->length(), ") was not equal to the length (",
 array.length(), ") multiplied by the list size (", array.value_length(), ")");
 }

 return Status::OK();
 }

 Status Visit(const StructArray& array) {
 if (array.length() < 0) {
 return Status::Invalid("Length was negative");
Expand Down

37 changes: 37 additions & 0 deletions
cpp/src/arrow/array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -522,6 +522,43 @@ class ARROW_EXPORT ListArray : public Array {
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// FixedSizeListArray

/// Concrete Array class for fixed size list data
class ARROW_EXPORT FixedSizeListArray : public Array {
@fsaintjacquesfsaintjacquesMay 9, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

is it worth having common abstract class?
@bkietzbkietzMay 9, 2019

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

There's not much opportunity for code reuse in arrays/builders. The only advantage I can think of would be easier reuse of code which consumes ListArray (but only code which doesn't directly access the offsets buffer). Those algorithms could use ArrayData BasicListArray::GetView(int64_t i) or similar. This seems like speculative generality to me; I'd be more open to doing it if you can think of existing code which would benefit from being refactored this way.
FixedSizeListScalar and ListScalar are identical but also trivial.
@pitroupitrouMay 9, 2019

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.

Agreed with @bkietz. Also code reuse can be achieved through templating (we generally care about performing non-virtual calls anyway).
@fsaintjacquesfsaintjacquesMay 10, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

👍
 public:
 using TypeClass = FixedSizeListType;

 explicit FixedSizeListArray(const std::shared_ptr<ArrayData>& data);

 FixedSizeListArray(const std::shared_ptr<DataType>& type, int64_t length,
 const std::shared_ptr<Array>& values,
 const std::shared_ptr<Buffer>& null_bitmap = NULLPTR,
 int64_t null_count = kUnknownNullCount, int64_t offset = 0);

 const FixedSizeListType* list_type() const;

 /// \brief Return array object containing the list's values
 std::shared_ptr<Array> values() const;

 std::shared_ptr<DataType> value_type() const;

 // Neither of these functions will perform boundschecking
 int32_t value_offset(int64_t i) const {
 i += data_->offset;
 return static_cast<int32_t>(list_size_ * i);
 }
 int32_t value_length(int64_t i = 0) const { return list_size_; }

 protected:
 void SetData(const std::shared_ptr<ArrayData>& data);
 int32_t list_size_;

 private:
 std::shared_ptr<Array> values_;
};

// ----------------------------------------------------------------------
// Binary and String

Expand Down
Loading