You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "HuaHuaY (via GitHub)" <gi...@apache.org> on 2026/06/12 09:31:19 UTC
[PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
HuaHuaY opened a new pull request, #50160:
URL: https://github.com/apache/arrow/pull/50160
### Rationale for this change
Allow us to write an arrow list view array into a parquet file.
### What changes are included in this PR?
We will sort the list views based on the offsets before inserting the array in Parquet writer, and generate a size array in Parquet reader.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419239719
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
Review Comment:
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3458631725
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
Review Comment:
Sorry that my previous reply was inaccurate. If this PR is merged, test cases in #35697 will not report `NotImplemented: Lists with non-zero length null components are not supported` but still report other issues. Null values ββin a `FixedSizeList` also need to occupy space, but they do not appear in the Parquet data array. So round trip tests will fail. We can fix `FixedSizeListReader` in future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3411441523
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -673,13 +673,39 @@ class ListReader : public ColumnReaderImpl {
const std::shared_ptr<Field> field() override { return field_; }
- private:
+ protected:
std::shared_ptr<ReaderContext> ctx_;
+
+ private:
std::shared_ptr<Field> field_;
::parquet::internal::LevelInfo level_info_;
std::unique_ptr<ColumnReaderImpl> item_reader_;
};
+template <typename IndexType>
+class PARQUET_NO_EXPORT ListViewReader : public ListReader<IndexType> {
+ public:
+ using ListReader<IndexType>::ListReader;
+
+ ::arrow::Result<std::shared_ptr<ChunkedArray>> AssembleArray(
+ std::shared_ptr<ArrayData> data) final {
+ DCHECK_EQ(data->buffers.size(), 2);
+ const auto* offsets = reinterpret_cast<const IndexType*>(data->buffers[1]->data());
+ ARROW_ASSIGN_OR_RAISE(
+ auto sizes_buffer,
+ AllocateResizableBuffer(sizeof(IndexType) * data->length, this->ctx_->pool));
+ auto* sizes = reinterpret_cast<IndexType*>(sizes_buffer->mutable_data());
+ for (int64_t i = 0; i < data->length; ++i) {
+ sizes[i] = offsets[i + 1] - offsets[i];
+ }
+ data->buffers[1] = ::arrow::SliceBuffer(std::move(data->buffers[1]), 0,
Review Comment:
Why do we need this? If it is really needed, let's add `/*offset=*/0` for better readability.
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -673,13 +673,39 @@ class ListReader : public ColumnReaderImpl {
const std::shared_ptr<Field> field() override { return field_; }
- private:
+ protected:
std::shared_ptr<ReaderContext> ctx_;
+
+ private:
std::shared_ptr<Field> field_;
::parquet::internal::LevelInfo level_info_;
std::unique_ptr<ColumnReaderImpl> item_reader_;
};
+template <typename IndexType>
+class PARQUET_NO_EXPORT ListViewReader : public ListReader<IndexType> {
+ public:
+ using ListReader<IndexType>::ListReader;
+
+ ::arrow::Result<std::shared_ptr<ChunkedArray>> AssembleArray(
+ std::shared_ptr<ArrayData> data) final {
+ DCHECK_EQ(data->buffers.size(), 2);
Review Comment:
Do we want to call `DCHECK_EQ` on `field()->type()->id()` just in case?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int64(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int64(), "[2, 1, 0, 2]");
+ auto element = ::arrow::field("element", ::arrow::int32());
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::LargeListViewArray::FromArrays(
+ ::arrow::large_list_view(element), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
Review Comment:
I think we need to add a comment somewhere to note that reading back into `ListView/LargeListView` is only supported from a file with `store_schema` enabled.
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,20 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
Review Comment:
Please add a comment to explain when `visited_component_size` will be greater than 1?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
Can we use a single test by parameterizing `schema` and `table` to reduce duplicated lines? I don't remember if `RecordBatchFromJson` supports list view natively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419048170
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I'm not sure why the child array would have a different name vs. the regular ListView case, but in any case you can have a helper function that also takes the inner element's name as argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3473389901
##########
cpp/src/parquet/arrow/path_internal.cc:
##########
@@ -387,17 +387,16 @@ class ListPathNode {
PathWriteContext* context) {
// First fill int the remainder of the list.
RETURN_IF_ERROR(FillRepLevels(child_range->Size(), rep_level_, context));
+
// Once we've reached this point the following preconditions should hold:
// 1. There are no more repeated path nodes to deal with.
- // 2. All elements in |range| represent contiguous elements in the
- // child array (Null values would have shortened the range to ensure
- // all remaining list elements are present (though they may be empty lists)).
+ // 2. Null values would have shortened the range to ensure all remaining
+ // list elements are present (though they may be empty lists).
// 3. No element of range spans a parent list (intermediate
// list nodes only handle one list entry at a time).
//
- // Given these preconditions it should be safe to fill runs on non-empty
+ // Given these preconditions it is safe to fill runs on contiguous non-empty
// lists here and expand the range in the child node accordingly.
-
while (!range->Empty()) {
ElementRange size_check = selector_.GetRange(range->start);
Review Comment:
I always get confused when seeing this `size_check`. Can we rename it to `next_child_range` (is my understand right)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419369272
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
+ ::arrow::ArrayVector arrays;
+ arrays.reserve(visited_component_size);
+ for (const auto& range : result.post_list_visited_elements) {
+ DCHECK(!range.Empty());
+ arrays.push_back(result.leaf_array->Slice(range.start, range.Size()));
+ }
+ ARROW_ASSIGN_OR_RAISE(values_array,
+ ::arrow::Concatenate(arrays, ctx->memory_pool));
Review Comment:
Oh, yes, that's a good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50160:
URL: https://github.com/apache/arrow/pull/50160#issuecomment-4804843312
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit cffe22d6fe7f73836d2d9177b4b4fb2d1fcf4d1f.
There were no benchmark performance regressions. π
The [full Conbench report](https://github.com/apache/arrow/runs/83554926543) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3458966970
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
Review Comment:
I think this is tested by `expected_array`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3473607571
##########
cpp/src/parquet/arrow/path_internal.cc:
##########
@@ -387,17 +387,16 @@ class ListPathNode {
PathWriteContext* context) {
// First fill int the remainder of the list.
RETURN_IF_ERROR(FillRepLevels(child_range->Size(), rep_level_, context));
+
// Once we've reached this point the following preconditions should hold:
// 1. There are no more repeated path nodes to deal with.
- // 2. All elements in |range| represent contiguous elements in the
- // child array (Null values would have shortened the range to ensure
- // all remaining list elements are present (though they may be empty lists)).
+ // 2. Null values would have shortened the range to ensure all remaining
+ // list elements are present (though they may be empty lists).
// 3. No element of range spans a parent list (intermediate
// list nodes only handle one list entry at a time).
//
- // Given these preconditions it should be safe to fill runs on non-empty
+ // Given these preconditions it is safe to fill runs on contiguous non-empty
// lists here and expand the range in the child node accordingly.
-
while (!range->Empty()) {
ElementRange size_check = selector_.GetRange(range->start);
Review Comment:
Yes. I have renamed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50160:
URL: https://github.com/apache/arrow/pull/50160#issuecomment-4798581488
Oh, I forgot two things, sorry:
1) we should update `ApplyOriginalStorageMetadata` in `parquet/arrow/schema.cc` so that it also recurses into list-view types
2) we should update the docs in https://arrow.apache.org/docs/cpp/parquet.html#types too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419343175
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
+ ::arrow::ArrayVector arrays;
+ arrays.reserve(visited_component_size);
+ for (const auto& range : result.post_list_visited_elements) {
+ DCHECK(!range.Empty());
+ arrays.push_back(result.leaf_array->Slice(range.start, range.Size()));
+ }
+ ARROW_ASSIGN_OR_RAISE(values_array,
+ ::arrow::Concatenate(arrays, ctx->memory_pool));
Review Comment:
It's difficult to write data without constructing a new array because we don't know the definition level and repetition level ranges for each segment of the array. Maybe we can add two offsets in `ElementRange` to implement this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3412147846
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I don't think there's much duplicate code here, and it's not easy to merge. Because without enabling `store_schema`, large lists require manually changing the inner array name from `item` to `element` within their type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3412147846
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I don't think there's much duplicate code here, and it's not easy to merge. Because without enabling `store_schema`, large list type requires manually changing the inner array name from `item` to `element`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3412200283
##########
cpp/src/parquet/properties.h:
##########
@@ -1374,7 +1374,11 @@ class PARQUET_EXPORT ArrowWriterProperties {
/// \brief EXPERIMENTAL: Write binary serialized Arrow schema to the file,
/// to enable certain read options (like "read_dictionary") to be set
- /// automatically
+ /// automatically.
Review Comment:
What about just modifying the original text to:
```
/// \brief EXPERIMENTAL: Write binary serialized Arrow schema to the file,
/// to enable certain read options (like "read_dictionary") to be set
/// automatically or read back non-default Arrow types like ListView,
/// LargeListView, LargeList, and Arrow extension types.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on PR #50160:
URL: https://github.com/apache/arrow/pull/50160#issuecomment-4778483318
@pitrou Sorry for the delay in updating. I have updated the code and comments. Please take a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50160:
URL: https://github.com/apache/arrow/pull/50160#issuecomment-4798797146
> Do you mean we should modify `GetNestedFactory`? I've already modified it before.
Oops, I had already forgotten about that. Sorry!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #50160:
URL: https://github.com/apache/arrow/pull/50160
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3412147846
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I don't think there's much duplicate code here, and it's not easy to merge. Because without enabling `store_schema`, large list type require manually changing the inner array name from `item` to `element` within their type.
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I don't think there's much duplicate code here, and it's not easy to merge. Because without enabling `store_schema`, large list type require manually changing the inner array name from `item` to `element`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3412147846
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I think there's not much duplicate code, so it's acceptable for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419373685
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
+ ::arrow::ArrayVector arrays;
+ arrays.reserve(visited_component_size);
+ for (const auto& range : result.post_list_visited_elements) {
+ DCHECK(!range.Empty());
+ arrays.push_back(result.leaf_array->Slice(range.start, range.Size()));
+ }
+ ARROW_ASSIGN_OR_RAISE(values_array,
+ ::arrow::Concatenate(arrays, ctx->memory_pool));
Review Comment:
> Maybe we can add two offsets in `ElementRange` to implement this.
We can, but perhaps that can be left as a later optimization.
(also, not sure it's a good idea if there are many `ElementRange`s in the first place)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419343175
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
+ ::arrow::ArrayVector arrays;
+ arrays.reserve(visited_component_size);
+ for (const auto& range : result.post_list_visited_elements) {
+ DCHECK(!range.Empty());
+ arrays.push_back(result.leaf_array->Slice(range.start, range.Size()));
+ }
+ ARROW_ASSIGN_OR_RAISE(values_array,
+ ::arrow::Concatenate(arrays, ctx->memory_pool));
Review Comment:
It's difficult to write data without constructing a new array because we don't know the definition level and repetition level ranges for each segment of the array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on PR #50160:
URL: https://github.com/apache/arrow/pull/50160#issuecomment-4798726470
> we should update ApplyOriginalStorageMetadata in parquet/arrow/schema.cc so that it also recurses into list-view types
Do you mean we should modify GetNestedFactory? I've already modified it before.
> we should update the docs in https://arrow.apache.org/docs/cpp/parquet.html#types too
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3473331409
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
Review Comment:
Oh, you're right, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3419189024
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
+ ::arrow::ArrayVector arrays;
+ arrays.reserve(visited_component_size);
+ for (const auto& range : result.post_list_visited_elements) {
+ DCHECK(!range.Empty());
+ arrays.push_back(result.leaf_array->Slice(range.start, range.Size()));
+ }
+ ARROW_ASSIGN_OR_RAISE(values_array,
+ ::arrow::Concatenate(arrays, ctx->memory_pool));
Review Comment:
Instead of concatenating, would it be possible (and potentially more efficient) to call `WriteArrow` for each individual chunk?
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
Review Comment:
Is this somehow fixing https://github.com/apache/arrow/issues/35697?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
Review Comment:
Can we also check the expect List result when `store_schema` is false? This would further validate that the actual Parquet contents are ok.
##########
cpp/src/parquet/arrow/path_internal.cc:
##########
@@ -406,14 +405,20 @@ class ListPathNode {
// def_levels entered first.
break;
}
+ if constexpr (RangeSelector::kCheckContiguous) {
+ if (size_check.start != child_range->end) {
Review Comment:
Can you comment on why we're doing this? This code is complicated and this will help the reader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3458631725
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
Review Comment:
Sorry that my previous reply was inaccurate. If this PR is merged, test cases in #35697 will not report `NotImplemented: Lists with non-zero length null components are not supported` but there are still other issues. Null values ββin a `FixedSizeList` also need to occupy space, but they do not appear in the Parquet data array. So round trip tests will fail. We can fix `FixedSizeListReader` in future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3458631725
##########
cpp/src/parquet/arrow/writer.cc:
##########
@@ -169,13 +170,24 @@ class ArrowColumnWriterV2 {
leaf_idx, ctx, [&](const MultipathLevelBuilderResult& result) {
size_t visited_component_size = result.post_list_visited_elements.size();
DCHECK_GT(visited_component_size, 0);
- if (visited_component_size != 1) {
- return Status::NotImplemented(
- "Lists with non-zero length null components are not supported");
+ std::shared_ptr<Array> values_array;
+ if (visited_component_size == 1) {
+ const ElementRange& range = result.post_list_visited_elements[0];
+ values_array = result.leaf_array->Slice(range.start, range.Size());
+ } else {
+ // Multiple leaf ranges can be produced when child values are
+ // skipped, such as null fixed-size-list slots, or when
+ // list-view ranges are non-contiguous. Concatenate the slices
+ // in logical write order.
Review Comment:
Sorry that my previous reply was inaccurate. If this PR is merged, test cases in #35697 will not report `NotImplemented: Lists with non-zero length null components are not supported` but there are still other issues. Null values ββin a `FixedSizeList` also need to occupy space, but they do not appear in the Parquet data array. So round trip tests will fail. We can fix #35697 in future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38849: [C++][Parquet] Add support for list view and large list view [arrow]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #50160:
URL: https://github.com/apache/arrow/pull/50160#discussion_r3459150585
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -3324,6 +3325,75 @@ TEST(ArrowReadWrite, LargeList) {
}
}
+TEST(ArrowReadWrite, ListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
+ auto offsets = ArrayFromJSON(::arrow::int32(), "[3, 0, 5, 1]");
+ auto sizes = ArrayFromJSON(::arrow::int32(), "[2, 1, 0, 2]");
+ ASSERT_OK_AND_ASSIGN(auto array, ::arrow::ListViewArray::FromArrays(
+ ::arrow::list_view(::arrow::int32()), *offsets,
+ *sizes, *values, default_memory_pool()));
+ auto table = Table::Make(
+ ::arrow::schema({::arrow::field("root", array->type(), false)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ CheckSimpleRoundtrip(table, 2, props_store_schema);
+
+ ASSERT_OK_AND_ASSIGN(auto expected_array,
+ ::arrow::ListArray::FromListView(*array, default_memory_pool()));
+ auto expected = Table::Make(
+ ::arrow::schema({::arrow::field("root", ::arrow::list(::arrow::int32()), false)}),
+ {expected_array});
+ CheckConfiguredRoundtrip(table, expected);
+}
+
+TEST(ArrowReadWrite, EmptyListView) {
+ auto type = ::arrow::list_view(::arrow::int32());
+ auto array = ArrayFromJSON(type, "[]");
+ auto table = Table::Make(::arrow::schema({::arrow::field("root", type)}), {array});
+
+ auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
+ std::shared_ptr<Table> result;
+ ASSERT_NO_FATAL_FAILURE(
+ DoRoundtrip(table, 1, &result, default_writer_properties(), props_store_schema));
+ ASSERT_OK(result->ValidateFull());
+
+ ASSERT_EQ(1, result->column(0)->num_chunks());
+ const auto& list_view =
+ checked_cast<const ::arrow::ListViewArray&>(*result->column(0)->chunk(0));
+ ASSERT_EQ(0, list_view.length());
+ ASSERT_NE(nullptr, list_view.value_offsets());
+ ASSERT_EQ(0, list_view.value_offsets()->size());
+ ASSERT_NE(nullptr, list_view.value_sizes());
+ ASSERT_EQ(0, list_view.value_sizes()->size());
+}
+
+TEST(ArrowReadWrite, LargeListView) {
+ auto values = ArrayFromJSON(::arrow::int32(), "[1, 2, 3, 4, 5]");
Review Comment:
I have updated the test code. I made a mistake that inner array name does not need to be changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org