Repository navigation
feat(ipc): Don't re-emit unchanged dictionaries when writing streams; fix nested dictionary and file writing #928
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
a34ce84
e5922b3
ad860f9
72308d3
24b8617
c673c8b
3e4046a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,7 +214,7 @@ class TestFile { | |
| } | ||
|
|
||
| ArrowErrorCode WriteNanoarrowStream(const nanoarrow::UniqueSchema& schema, | ||
| const std::vector<nanoarrow::UniqueArray>& arrays, | ||
| std::vector<nanoarrow::UniqueArray>& arrays, | ||
| enum ArrowIpcCompressionType codec, | ||
| struct ArrowBuffer* buffer, | ||
| struct ArrowError* error) { | ||
|
|
@@ -226,19 +226,25 @@ class TestFile { | |
| NANOARROW_RETURN_NOT_OK(ArrowIpcWriterSetCompression( | ||
| writer.get(), codec, NANOARROW_IPC_COMPRESSION_LEVEL_DEFAULT, error)); | ||
|
|
||
| nanoarrow::UniqueArrayView array_view; | ||
| nanoarrow::UniqueSchema schema_copy; | ||
| NANOARROW_RETURN_NOT_OK(ArrowSchemaDeepCopy(schema.get(), schema_copy.get())); | ||
| nanoarrow::UniqueArrayStream array_stream; | ||
| NANOARROW_RETURN_NOT_OK( | ||
| ArrowArrayViewInitFromSchema(array_view.get(), schema.get(), error)); | ||
|
|
||
| NANOARROW_RETURN_NOT_OK(ArrowIpcWriterWriteSchema(writer.get(), schema.get(), error)); | ||
| for (const auto& array : arrays) { | ||
| NANOARROW_RETURN_NOT_OK( | ||
| ArrowArrayViewSetArray(array_view.get(), array.get(), error)); | ||
|
|
||
| NANOARROW_RETURN_NOT_OK( | ||
| ArrowIpcWriterWriteArrayView(writer.get(), array_view.get(), error)); | ||
| ArrowBasicArrayStreamInit(array_stream.get(), schema_copy.get(), arrays.size())); | ||
|
|
||
| for (size_t i = 0; i < arrays.size(); i++) { | ||
| // Preserve the decoded array for the subsequent Arrow C++ comparison while | ||
| // giving the basic stream an independently releasable shared clone. | ||
| nanoarrow::UniqueArray shared; | ||
| nanoarrow::UniqueArray clone; | ||
| NANOARROW_RETURN_NOT_OK(ArrowArrayMoveShared(arrays[i].get(), shared.get())); | ||
| ArrowErrorCode result = ArrowArrayCloneShared(shared.get(), clone.get()); | ||
| ArrowArrayMove(shared.get(), arrays[i].get()); | ||
| NANOARROW_RETURN_NOT_OK(result); | ||
| ArrowBasicArrayStreamSetArray(array_stream.get(), i, clone.get()); | ||
| } | ||
| return ArrowIpcWriterWriteArrayView(writer.get(), nullptr, error); | ||
|
|
||
| return ArrowIpcWriterWriteArrayStream(writer.get(), array_stream.get(), error); | ||
| } | ||
|
|
||
| void TestEqualsArrowCpp(const std::string& dir_prefix, | ||
|
|
@@ -529,10 +535,10 @@ INSTANTIATE_TEST_SUITE_P( | |
| TestFile::OK("generated_primitive.stream"), | ||
| TestFile::OK("generated_recursive_nested.stream"), | ||
| TestFile::OK("generated_union.stream"), | ||
| TestFile::ReadOnly("generated_dictionary_unsigned.stream"), | ||
| TestFile::ReadOnly("generated_dictionary.stream"), | ||
| TestFile::ReadOnly("generated_nested_dictionary.stream"), | ||
| TestFile::ReadOnly("generated_extension.stream") | ||
| TestFile::OK("generated_dictionary_unsigned.stream"), | ||
| TestFile::OK("generated_dictionary.stream"), | ||
| TestFile::OK("generated_nested_dictionary.stream"), | ||
| TestFile::OK("generated_extension.stream") | ||
| // Comment to keep last line from wrapping | ||
| )); | ||
|
|
||
|
|
@@ -581,7 +587,7 @@ INSTANTIATE_TEST_SUITE_P( | |
| TestFile::OK("0.14.1/generated_primitive.stream"), | ||
| TestFile::OK("0.14.1/generated_primitive_no_batches.stream"), | ||
| TestFile::OK("0.14.1/generated_primitive_zerolength.stream"), | ||
| TestFile::ReadOnly("4.0.0-shareddict/generated_shared_dict.stream"), | ||
| TestFile::OK("4.0.0-shareddict/generated_shared_dict.stream"), | ||
| // cpp-21.0.0 regenerated gold files | ||
| TestFile::OK("cpp-21.0.0/generated_binary.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_binary_no_batches.stream"), | ||
|
|
@@ -608,10 +614,10 @@ INSTANTIATE_TEST_SUITE_P( | |
| TestFile::OK("cpp-21.0.0/generated_primitive_zerolength.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_recursive_nested.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_union.stream"), | ||
| TestFile::ReadOnly("cpp-21.0.0/generated_dictionary.stream"), | ||
| TestFile::ReadOnly("cpp-21.0.0/generated_dictionary_unsigned.stream"), | ||
| TestFile::ReadOnly("cpp-21.0.0/generated_extension.stream"), | ||
| TestFile::ReadOnly("cpp-21.0.0/generated_nested_dictionary.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_dictionary.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_dictionary_unsigned.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_extension.stream"), | ||
| TestFile::OK("cpp-21.0.0/generated_nested_dictionary.stream"), | ||
|
Comment on lines
+617
to
+620
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious: what feature is needed to roundtrip the generated extension beyond what's here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing beyond this PR: the file's |
||
| TestFile::NotSupported("cpp-21.0.0/generated_list_view.stream"), | ||
| TestFile::NotSupported("cpp-21.0.0/generated_binary_view.stream"), | ||
| TestFile::NotSupported("cpp-21.0.0/generated_run_end_encoded.stream") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great and super useful change, but a separate one from the decoding or encoding of delta dictionaries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. It stays in this PR, which is now only the non-delta writer and encoder changes.