From fa231a51c7a0e6ff884e8b3b149edc9184279a51 Mon Sep 17 00:00:00 2001 From: JonSnow1807 Date: Thu, 22 Oct 2026 11:49:13 -0500 Subject: [PATCH] fix: Compare metadata of nested child fields in TestingJSONComparison CompareFieldBase() serializes both fields without metadata and then compares the metadata of the top-level field only, so a consumer that drops the metadata of a child of a struct, list, map or union field (e.g., an extension type on a list item) still passes the comparison. Recurse through the children of each field (and the children of a dictionary value type) and compare their metadata at ".children[i].metadata". Empty and absent metadata are treated as equal there because IPC and the integration JSON both omit empty metadata. Fixes #973 --- src/nanoarrow/nanoarrow_testing.hpp | 4 + src/nanoarrow/testing/testing.cc | 50 ++++++++++++ src/nanoarrow/testing/testing_test.cc | 108 ++++++++++++++++++++++++++ 3 files changed, 162 insertions(+) diff --git a/src/nanoarrow/nanoarrow_testing.hpp b/src/nanoarrow/nanoarrow_testing.hpp index 945e19ea3..7b6d5f675 100644 --- a/src/nanoarrow/nanoarrow_testing.hpp +++ b/src/nanoarrow/nanoarrow_testing.hpp @@ -278,6 +278,10 @@ class NANOARROW_DLL TestingJSONComparison { ArrowErrorCode CompareMetadata(const char* actual, const char* expected, ArrowError* error, const std::string& path = ""); + ArrowErrorCode CompareChildrenMetadata(const ArrowSchema* actual, + const ArrowSchema* expected, ArrowError* error, + const std::string& path = ""); + ArrowErrorCode MetadataEqualKeyValue(const char* actual, const char* expected, bool* out, ArrowError* error); diff --git a/src/nanoarrow/testing/testing.cc b/src/nanoarrow/testing/testing.cc index df69cebe5..1b481c9d7 100644 --- a/src/nanoarrow/testing/testing.cc +++ b/src/nanoarrow/testing/testing.cc @@ -2778,6 +2778,56 @@ ArrowErrorCode TestingJSONComparison::CompareFieldBase(ArrowSchema* actual, NANOARROW_RETURN_NOT_OK(CompareMetadata(actual->metadata, expected->metadata, error, path + std::string(".metadata"))); + NANOARROW_RETURN_NOT_OK(CompareChildrenMetadata(actual, expected, error, path)); + return NANOARROW_OK; +} + +namespace { + +// Empty metadata is equivalent to no metadata (e.g., both are omitted when written to +// IPC or to the integration JSON) +ArrowErrorCode MetadataOrNullIfEmpty(const char* metadata, const char** out) { + ArrowMetadataReader reader; + NANOARROW_RETURN_NOT_OK(ArrowMetadataReaderInit(&reader, metadata)); + *out = reader.remaining_keys > 0 ? metadata : nullptr; + return NANOARROW_OK; +} + +} // namespace + +ArrowErrorCode TestingJSONComparison::CompareChildrenMetadata(const ArrowSchema* actual, + const ArrowSchema* expected, + ArrowError* error, + const std::string& path) { + // The serialized field (which does not include metadata) was already compared by + // the caller and records any difference in structure (e.g., the number of children + // or the presence of a dictionary), so only the metadata of children that exist in + // both fields is compared here. + if (actual->n_children == expected->n_children) { + for (int64_t i = 0; i < expected->n_children; i++) { + std::string child_path = path + ".children[" + std::to_string(i) + "]"; + + const char* actual_metadata; + const char* expected_metadata; + NANOARROW_RETURN_NOT_OK_WITH_ERROR( + MetadataOrNullIfEmpty(actual->children[i]->metadata, &actual_metadata), error); + NANOARROW_RETURN_NOT_OK_WITH_ERROR( + MetadataOrNullIfEmpty(expected->children[i]->metadata, &expected_metadata), + error); + NANOARROW_RETURN_NOT_OK(CompareMetadata(actual_metadata, expected_metadata, error, + child_path + ".metadata")); + + NANOARROW_RETURN_NOT_OK(CompareChildrenMetadata( + actual->children[i], expected->children[i], error, child_path)); + } + } + + // The children of a dictionary-encoded field are those of the dictionary value type + if (actual->dictionary != nullptr && expected->dictionary != nullptr) { + NANOARROW_RETURN_NOT_OK(CompareChildrenMetadata( + actual->dictionary, expected->dictionary, error, path + ".dictionary")); + } + return NANOARROW_OK; } diff --git a/src/nanoarrow/testing/testing_test.cc b/src/nanoarrow/testing/testing_test.cc index 8c3cb4c36..1f3ccb7e1 100644 --- a/src/nanoarrow/testing/testing_test.cc +++ b/src/nanoarrow/testing/testing_test.cc @@ -1655,6 +1655,114 @@ TEST(NanoarrowTestingTest, NanoarrowTestingTestMetadataComparison) { ASSERT_EQ(comparison.CompareSchema(actual.get(), expected.get()), ENOTSUP); } +TEST(NanoarrowTestingTest, NanoarrowTestingTestNestedMetadataComparison) { + nanoarrow::UniqueSchema actual; + nanoarrow::UniqueSchema expected; + nanoarrow::UniqueBuffer buf; + + ASSERT_EQ(ArrowMetadataBuilderInit(buf.get(), nullptr), NANOARROW_OK); + ASSERT_EQ(ArrowMetadataBuilderAppend(buf.get(), "key1"_asv, "value1"_asv), + NANOARROW_OK); + const char* metadata = reinterpret_cast(buf->data); + + // Start with two identical schemas whose list item has metadata + ArrowSchemaInit(actual.get()); + ASSERT_EQ(ArrowSchemaSetTypeStruct(actual.get(), 1), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0], NANOARROW_TYPE_LIST), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0]->children[0], NANOARROW_TYPE_INT32), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetMetadata(actual->children[0]->children[0], metadata), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaDeepCopy(actual.get(), expected.get()), NANOARROW_OK); + AssertSchemasCompareEqual(actual.get(), expected.get()); + + // With the metadata of the list item dropped from the actual schema + ASSERT_EQ(ArrowSchemaSetMetadata(actual->children[0]->children[0], nullptr), + NANOARROW_OK); + AssertSchemasCompareUnequal(actual.get(), expected.get(), /*num_differences*/ 1, + /*differences*/ + "Path: .children[0].children[0].metadata" + R"( +- null ++ [{"key": "key1", "value": "value1"}] + +)"); + + // ...also using the comparison that does *not* consider ordering + AssertSchemasCompareUnequal(actual.get(), expected.get(), /*num_differences*/ 1, + /*differences*/ + "Path: .children[0].children[0].metadata" + R"( +- null ++ [{"key": "key1", "value": "value1"}] + +)", + [](TestingJSONComparison& comparison) { + comparison.set_compare_metadata_order(false); + }); + + // Empty metadata on a child is equivalent to no metadata (IPC and the integration + // JSON both omit empty metadata) + ASSERT_EQ(ArrowSchemaSetMetadata(actual->children[0]->children[0], "\0\0\0\0"), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetMetadata(expected->children[0]->children[0], nullptr), + NANOARROW_OK); + AssertSchemasCompareEqual(actual.get(), expected.get()); + + // Metadata on the value child of a map is compared (the map's canonical names + // are forced before comparison, which must not affect the metadata) + actual.reset(); + expected.reset(); + ArrowSchemaInit(actual.get()); + ASSERT_EQ(ArrowSchemaSetTypeStruct(actual.get(), 1), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0], NANOARROW_TYPE_MAP), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0]->children[0]->children[0], + NANOARROW_TYPE_STRING), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0]->children[0]->children[1], + NANOARROW_TYPE_INT32), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaDeepCopy(actual.get(), expected.get()), NANOARROW_OK); + ASSERT_EQ( + ArrowSchemaSetMetadata(expected->children[0]->children[0]->children[1], metadata), + NANOARROW_OK); + AssertSchemasCompareUnequal(actual.get(), expected.get(), /*num_differences*/ 1, + /*differences*/ + "Path: .children[0].children[0].children[1].metadata" + R"( +- null ++ [{"key": "key1", "value": "value1"}] + +)"); + + // Metadata on a child of a dictionary value type is compared + actual.reset(); + expected.reset(); + ArrowSchemaInit(actual.get()); + ASSERT_EQ(ArrowSchemaSetTypeStruct(actual.get(), 1), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0], NANOARROW_TYPE_INT32), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaAllocateDictionary(actual->children[0]), NANOARROW_OK); + ArrowSchemaInit(actual->children[0]->dictionary); + ASSERT_EQ(ArrowSchemaSetTypeStruct(actual->children[0]->dictionary, 1), NANOARROW_OK); + ASSERT_EQ(ArrowSchemaSetType(actual->children[0]->dictionary->children[0], + NANOARROW_TYPE_INT32), + NANOARROW_OK); + ASSERT_EQ(ArrowSchemaDeepCopy(actual.get(), expected.get()), NANOARROW_OK); + AssertSchemasCompareEqual(actual.get(), expected.get()); + + ASSERT_EQ( + ArrowSchemaSetMetadata(expected->children[0]->dictionary->children[0], metadata), + NANOARROW_OK); + AssertSchemasCompareUnequal(actual.get(), expected.get(), /*num_differences*/ 1, + /*differences*/ + "Path: .children[0].dictionary.children[0].metadata" + R"( +- null ++ [{"key": "key1", "value": "value1"}] + +)"); +} + TEST(NanoarrowTestingTest, NanoarrowTestingTestArrayComparison) { nanoarrow::UniqueSchema schema; nanoarrow::UniqueArray actual;