Alb3e3 commented on code in PR #50927:
URL: https://github.com/apache/arrow/pull/50927#discussion_r3853910398
##########
cpp/src/arrow/ipc/read_write_test.cc:
##########
@@ -71,6 +71,73 @@ using MetadataVector =
std::vector<std::shared_ptr<KeyValueMetadata>>;
namespace test {
+class UnionExtensionArray : public ExtensionArray {
Review Comment:
Moved to `arrow/testing/extension_type.h` in `4c4ecb9390`, with the
`ExtensionEquals` / `MakeArray` / `Deserialize` bodies in
`testing/gtest_util.cc` next to the other example extension types.
I kept `UnionExtensionType` parameterized on `(storage_type,
extension_name)` rather than splitting it into two classes, so the dense- and
sparse-backed variants can be registered at the same time. Accessors are
`dense_union_extension_type()` and `sparse_union_extension_type()`, matching
the `dict_extension_type()` / `complex128()` naming. `Deserialize()` now also
validates the storage type against `storage_type_`, which the throwaway version
did not.
##########
cpp/src/arrow/ipc/read_write_test.cc:
##########
@@ -71,6 +71,73 @@ using MetadataVector =
std::vector<std::shared_ptr<KeyValueMetadata>>;
namespace test {
+class UnionExtensionArray : public ExtensionArray {
+ public:
+ using ExtensionArray::ExtensionArray;
+};
+
+class UnionExtensionType : public ExtensionType {
+ public:
+ UnionExtensionType(std::shared_ptr<DataType> storage_type, std::string
extension_name)
+ : ExtensionType(std::move(storage_type)),
+ extension_name_(std::move(extension_name)) {}
+
+ std::string extension_name() const override { return extension_name_; }
+
+ bool ExtensionEquals(const ExtensionType& other) const override {
+ return other.extension_name() == extension_name_;
+ }
+
+ std::shared_ptr<Array> MakeArray(std::shared_ptr<ArrayData> data) const
override {
+ return std::make_shared<UnionExtensionArray>(std::move(data));
+ }
+
+ Result<std::shared_ptr<DataType>> Deserialize(
+ std::shared_ptr<DataType> storage_type,
+ const std::string& serialized) const override {
+ if (serialized != extension_name_) {
+ return Status::Invalid("Type identifier did not match");
+ }
+ return std::make_shared<UnionExtensionType>(std::move(storage_type),
+ extension_name_);
+ }
+
+ std::string Serialize() const override { return extension_name_; }
+
+ private:
+ std::string extension_name_;
+};
+
+std::shared_ptr<DataType> dense_union_extension_type() {
+ return std::make_shared<UnionExtensionType>(
+ dense_union({field("floats", float64()), field("strings",
large_utf8())}, {0, 1}),
+ "dense-union-extension");
+}
+
+std::shared_ptr<DataType> sparse_union_extension_type() {
+ return std::make_shared<UnionExtensionType>(
+ sparse_union({field("floats", float64()), field("strings",
large_utf8())}, {0, 1}),
+ "sparse-union-extension");
+}
+
+Status MakeDenseUnionExtension(std::shared_ptr<RecordBatch>* out) {
Review Comment:
Moved to `arrow/ipc/test_common.{h,cc}` as `MakeDenseUnionExtension` /
`MakeSparseUnionExtension`, sitting right after `MakeDictExtension`. Both
delegate to a file-local `MakeUnionExtension(type, out)`.
While moving them I also made them match the shape of the neighbouring
makers — two fields (`f0` nullable, `f1` non-nullable) instead of one, and the
nullable one now carries a null (`[[0, 1.5], [1, null]]`), which the originals
did not exercise.
##########
cpp/src/arrow/ipc/read_write_test.cc:
##########
@@ -1954,6 +2023,22 @@ TEST_P(TestFileFormatGeneratorCoalesced, RoundTrip) {
TEST_P(TestStreamFormat, RoundTrip) { TestRoundTripWithOptions(*GetParam()); }
+TEST_F(TestFileFormat, DenseUnionExtensionRoundTrip) {
Review Comment:
Done — folded into `kBatchCases` and the four `TEST_F` blocks are gone.
Worth noting this is a strict improvement in coverage, not just tidier: the
two cases now run through **22** test instances instead of 4, and that picks up
`TestIpcRoundTrip.SliceRoundTrip` and `TestIpcRoundTrip.ZeroLengthArrays`, plus
the four `StreamDecoder*` variants and `TestFileFormatGenerator{,Coalesced}` —
none of which the dedicated tests touched.
It also made the regression sharper. Reverting `writer.cc` alone and
rerunning, the new cases fail in `TestFileFormat`,
`TestIpcRoundTrip.RoundTrip`, `TestIpcRoundTrip.ZeroLengthArrays`, and then
`TestFileFormatGenerator` aborts outright on an out-of-bounds `ArrayData` child
access.
##########
cpp/src/arrow/ipc/writer.cc:
##########
@@ -164,18 +170,19 @@ class RecordBatchSerializer {
// In V4, null types have no validity bitmap
// In V5 and later, null and union types have no validity bitmap
- if (internal::HasValidityBitmap(arr.type_id(), options_.metadata_version))
{
+ if (internal::HasValidityBitmap(physical_arr->type_id(),
options_.metadata_version)) {
if (arr.null_count() > 0) {
Review Comment:
Fair — that inconsistency was not deliberate, it was left over from patching
the call sites one at a time.
Rewritten to unwrap once into a reference and then use it everywhere:
```c++
const Array& physical_arr = arr.type_id() == Type::EXTENSION
? *checked_cast<const
ExtensionArray&>(arr).storage()
: arr;
```
`arr` is not referenced again after that line. This is safe because
`ExtensionArray::SetData()` builds the storage from `data->Copy()` with only
the type swapped, so `length`, `offset`, `null_count` and the buffers are
shared — the only thing that differs is the type id, which is exactly what the
layout decisions below need. Added a comment saying so.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
