From 69bc74a82ae8ca3ac233a483a07bc3c64780ddb8 Mon Sep 17 00:00:00 2001 From: Nicholas Jiang Date: Mon, 21 Sep 2026 19:26:16 +0800 Subject: [PATCH] fix(types): assign omitted field IDs in type JSON Accept configured Variant shredding schemas whose ROW fields all omit IDs, matching Java Paimon's preorder numbering from 0. Preserve explicit IDs and require them when deserializing stored table schemas. Reject partially specified IDs in either order. Java's current parser accepts an explicit ID followed by an omitted one, which can produce duplicate IDs; reject that case as required by #347. Share the assigner across nested ROW, ARRAY and MAP types, preserve field names and descriptions without truncation, and cover both configuration keys and Parquet round trips with physical field IDs. --- docs/source/user_guide/data_types.rst | 5 +- include/paimon/defs.h | 4 +- .../variant/variant_shredding_write_plan.h | 3 +- ...iant_shredding_write_plan_factory_test.cpp | 71 ++++++++-- src/paimon/common/types/data_field.cpp | 16 +-- src/paimon/common/types/data_field_test.cpp | 31 ++++- .../common/types/data_type_json_parser.cpp | 123 ++++++++++++++--- .../common/types/data_type_json_parser.h | 45 ++++++- .../types/data_type_json_parser_test.cpp | 127 ++++++++++++++++++ .../format/parquet/variant_parquet_test.cpp | 38 +++++- 10 files changed, 401 insertions(+), 62 deletions(-) diff --git a/docs/source/user_guide/data_types.rst b/docs/source/user_guide/data_types.rst index c1bdda4a0..17fdaca14 100644 --- a/docs/source/user_guide/data_types.rst +++ b/docs/source/user_guide/data_types.rst @@ -271,7 +271,10 @@ and `Arrow DataTypes `_ specification by setting ``variant.shreddingSchema`` to a ROW type JSON whose fields map top-level variant column names to their shredding - types. Alternatively, setting ``variant.inferShreddingSchema`` to + types. Explicit field IDs are preserved. If every ROW field, including + nested fields, omits ``id``, IDs are assigned in preorder starting at 0, + as in Java Paimon. Partially specified IDs are rejected. + Alternatively, setting ``variant.inferShreddingSchema`` to ``true`` infers a shredding schema per file from the first written rows (tuned by ``variant.shredding.maxSchemaWidth``, which bounds the total number of shredded fields across all variant columns of the schema, diff --git a/include/paimon/defs.h b/include/paimon/defs.h index f51eb4520..517ddb8b6 100644 --- a/include/paimon/defs.h +++ b/include/paimon/defs.h @@ -511,7 +511,9 @@ struct PAIMON_EXPORT Options { static const char MAP_SHARED_SHREDDING_COLUMN_PLACEMENT_POLICY[]; /// "variant.shreddingSchema" - The Variant shredding schema for writing: a ROW type JSON - /// whose fields map variant column names to their shredding types. No default value. + /// whose fields map variant column names to their shredding types. All ROW fields, including + /// nested fields, must all specify 'id' or all omit it. Omitted IDs are assigned in preorder + /// starting at 0. No default value. static const char VARIANT_SHREDDING_SCHEMA[]; /// "parquet.variant.shreddingSchema" - Fallback key of "variant.shreddingSchema". static const char PARQUET_VARIANT_SHREDDING_SCHEMA[]; diff --git a/src/paimon/common/data/variant/variant_shredding_write_plan.h b/src/paimon/common/data/variant/variant_shredding_write_plan.h index 2fa7a886e..c9903fe02 100644 --- a/src/paimon/common/data/variant/variant_shredding_write_plan.h +++ b/src/paimon/common/data/variant/variant_shredding_write_plan.h @@ -75,7 +75,8 @@ class VariantShreddingWritePlan { /// Creates a plan from the `variant.shreddingSchema` option value: a ROW type JSON whose /// fields map top-level variant column names to their shredding types (nested variant - /// columns cannot be configured, as in Java). + /// columns cannot be configured, as in Java). All ROW fields, including nested fields, must + /// all specify 'id' or all omit it. Omitted IDs are assigned in preorder starting at 0. static Result> FromConfiguredSchema( const std::shared_ptr& logical_schema, const std::string& configured_schema_json); diff --git a/src/paimon/common/data/variant/variant_shredding_write_plan_factory_test.cpp b/src/paimon/common/data/variant/variant_shredding_write_plan_factory_test.cpp index 1546acade..49b28134f 100644 --- a/src/paimon/common/data/variant/variant_shredding_write_plan_factory_test.cpp +++ b/src/paimon/common/data/variant/variant_shredding_write_plan_factory_test.cpp @@ -95,20 +95,63 @@ TEST_F(VariantShreddingWritePlanFactoryTest, ConfiguredSchema) { } } ] })"; - ASSERT_OK_AND_ASSIGN(CoreOptions options, - MakeOptions({{"variant.shreddingSchema", shredding_schema_json}})); - auto factory = VariantShreddingWritePlanFactory::Create(options, schema_, pool_); - ASSERT_TRUE(factory->ShouldCreateWritePlan()); - ASSERT_FALSE(factory->ShouldInferWritePlan()); - ASSERT_OK_AND_ASSIGN(std::shared_ptr converter, - factory->CreateConverter("parquet", {})); - ASSERT_NE(converter, nullptr); - auto variant_field = converter->GetPhysicalSchema()->GetFieldByName("v"); - ASSERT_NE(variant_field, nullptr); - const auto& physical_type = static_cast(*variant_field->type()); - ASSERT_NE(physical_type.GetFieldByName("typed_value"), nullptr); - // Variant shredding only supports the parquet format. - ASSERT_TRUE(factory->CreateConverter("orc", {}).status().IsNotImplemented()); + const char* shredding_schema_json_without_ids = R"({ + "type": "ROW", + "fields": [ { + "name": "v", + "type": { + "type": "ROW", + "fields": [ + {"name": "age", "type": "INT"}, + {"name": "city", "type": "STRING"} + ] + } + } ] + })"; + std::shared_ptr physical_schema; + for (const char* option_key : {"variant.shreddingSchema", "parquet.variant.shreddingSchema"}) { + SCOPED_TRACE(option_key); + for (const char* configured_schema : + {shredding_schema_json, shredding_schema_json_without_ids}) { + SCOPED_TRACE(configured_schema); + ASSERT_OK_AND_ASSIGN(CoreOptions options, + MakeOptions({{option_key, configured_schema}})); + auto factory = VariantShreddingWritePlanFactory::Create(options, schema_, pool_); + ASSERT_TRUE(factory->ShouldCreateWritePlan()); + ASSERT_FALSE(factory->ShouldInferWritePlan()); + ASSERT_OK_AND_ASSIGN(std::shared_ptr converter, + factory->CreateConverter("parquet", {})); + ASSERT_NE(converter, nullptr); + auto variant_field = converter->GetPhysicalSchema()->GetFieldByName("v"); + ASSERT_NE(variant_field, nullptr); + const auto& physical_type = + static_cast(*variant_field->type()); + ASSERT_NE(physical_type.GetFieldByName("typed_value"), nullptr); + if (physical_schema == nullptr) { + physical_schema = converter->GetPhysicalSchema(); + } else { + ASSERT_TRUE(converter->GetPhysicalSchema()->Equals(*physical_schema, + /*check_metadata=*/true)); + } + ASSERT_TRUE(factory->CreateConverter("orc", {}).status().IsNotImplemented()); + } + } +} + +TEST_F(VariantShreddingWritePlanFactoryTest, ConfiguredSchemaRejectsPartialFieldIds) { + for (const char* configured_schema : { + R"({"type":"ROW","fields":[{"id":0,"name":"v","type":{ + "type":"ROW","fields":[{"name":"age","type":"INT"}]}}]})", + R"({"type":"ROW","fields":[{"name":"v","type":{ + "type":"ROW","fields":[{"id":1,"name":"age","type":"INT"}]}}]})", + }) { + SCOPED_TRACE(configured_schema); + ASSERT_OK_AND_ASSIGN(CoreOptions options, + MakeOptions({{"variant.shreddingSchema", configured_schema}})); + auto factory = VariantShreddingWritePlanFactory::Create(options, schema_, pool_); + ASSERT_NOK_WITH_MSG(factory->CreateConverter("parquet", {}), + "Partial field id is not allowed."); + } } TEST_F(VariantShreddingWritePlanFactoryTest, InferredSchema) { diff --git a/src/paimon/common/types/data_field.cpp b/src/paimon/common/types/data_field.cpp index 97a167b0d..8323fcf0b 100644 --- a/src/paimon/common/types/data_field.cpp +++ b/src/paimon/common/types/data_field.cpp @@ -65,21 +65,13 @@ rapidjson::Value DataField::ToJson(rapidjson::Document::AllocatorType* allocator } void DataField::FromJson(const rapidjson::Value& obj) noexcept(false) { - id_ = RapidJsonUtil::DeserializeKeyValue(obj, "id"); - auto name = RapidJsonUtil::DeserializeKeyValue(obj, "name"); - assert(obj.IsObject()); - if (!obj.HasMember("type")) { - throw std::invalid_argument("key 'type' must exist"); - } - auto field_result = DataTypeJsonParser::ParseType(name, obj["type"]); + // Serialized table schemas require explicit IDs for projection and schema evolution. + Result field_result = DataTypeJsonParser::ParseDataField(obj); if (!field_result.ok()) { - throw std::invalid_argument( - fmt::format("parse data type failed, error msg: {}", field_result.status().ToString())); + throw std::invalid_argument(field_result.status().ToString()); } - field_ = field_result.value(); + *this = std::move(field_result).value(); assert(field_); - description_ = RapidJsonUtil::DeserializeKeyValue>( - obj, "description", description_); } std::shared_ptr DataField::ConvertDataFieldToArrowField(const DataField& field) { diff --git a/src/paimon/common/types/data_field_test.cpp b/src/paimon/common/types/data_field_test.cpp index 6ac52ff59..27841aaa9 100644 --- a/src/paimon/common/types/data_field_test.cpp +++ b/src/paimon/common/types/data_field_test.cpp @@ -20,6 +20,7 @@ #include "paimon/common/types/data_field.h" #include +#include #include "arrow/api.h" #include "gtest/gtest.h" @@ -137,7 +138,8 @@ TEST_F(DataFieldTest, ConvertArrowFieldToDataField) { TEST_F(DataFieldTest, FromJson) { const char* json = R"({ "id" : 0, - "name" : "f0", + "name" : "f0\u0000tail", + "description" : "d\u0000tail", "type" : { "type" : "ROW", "fields" : [ { @@ -161,7 +163,8 @@ TEST_F(DataFieldTest, FromJson) { DataField field; field.FromJson(doc); EXPECT_EQ(field.Id(), 0); - EXPECT_EQ(field.Name(), "f0"); + ASSERT_EQ(field.Name(), std::string("f0\0tail", 7)); + ASSERT_EQ(field.Description(), std::string("d\0tail", 6)); EXPECT_EQ(field.Type()->id(), arrow::Type::STRUCT); auto sub_fields = field.Type()->fields(); @@ -226,6 +229,30 @@ TEST_F(DataFieldTest, FromJsonFailed) { })"; check_result(json_str, "parse data type failed, error msg: "); } + const std::vector> test_cases = { + {R"([{"id":0,"name":"a","type":"INT"}])", "data field must be an object"}, + {R"({"id":"0","name":"a","type":"INT"})", "value of key 'id' must be int"}, + {R"({"name":"a","type":"INT"})", "key 'id' must exist"}, + {R"({"id":null,"name":"a","type":"INT"})", "key 'id' must exist"}, + {R"({"id":0,"type":"INT"})", "key 'name' must exist"}, + {R"({"id":0,"name":0,"type":"INT"})", "value of key 'name' must be string"}, + {R"({"id":0,"name":"a"})", "key 'type' must exist"}, + {R"({"id":0,"name":"a","type":"INT","description":0})", + "value of key 'description' must be string"}, + {R"({"id":0,"name":"a","type":{"type":"ROW", + "fields":[{"name":"b","type":"INT"}]}})", + "key 'id' must exist"}, + {R"({"id":0,"name":"a","type":{"type":"ARRAY","element":{"type":"ROW", + "fields":[{"name":"b","type":"INT"}]}}})", + "key 'id' must exist"}, + {R"({"id":0,"name":"a","type":{"type":"MAP","key":"STRING", + "value":{"type":"ROW","fields":[{"name":"b","type":"INT"}]}}})", + "key 'id' must exist"}, + }; + for (const auto& [json, error_msg] : test_cases) { + SCOPED_TRACE(json); + ASSERT_NO_FATAL_FAILURE(check_result(json, error_msg)); + } } TEST_F(DataFieldTest, ToJson) { diff --git a/src/paimon/common/types/data_type_json_parser.cpp b/src/paimon/common/types/data_type_json_parser.cpp index d04e5ade5..adcf5b73a 100644 --- a/src/paimon/common/types/data_type_json_parser.cpp +++ b/src/paimon/common/types/data_type_json_parser.cpp @@ -35,7 +35,6 @@ #include "paimon/common/types/data_field.h" #include "paimon/common/types/vector_type.h" #include "paimon/common/utils/date_time_utils.h" -#include "paimon/common/utils/rapidjson_util.h" #include "paimon/common/utils/string_utils.h" #include "paimon/data/decimal.h" #include "paimon/data/timestamp.h" @@ -647,17 +646,93 @@ Result TokenParser::ParseOptionalPrecision(int32_t default_precision) { } } // namespace +Result DataTypeJsonParser::FieldIdAssigner::Assign( + const std::optional& explicit_id) { + // Mixing both forms would let a generated id collide with an explicit one. + if (explicit_id.has_value()) { + if (next_generated_id_ != 0) { + return Status::Invalid("Partial field id is not allowed."); + } + has_explicit_id_ = true; + return explicit_id.value(); + } + if (has_explicit_id_) { + return Status::Invalid("Partial field id is not allowed."); + } + return next_generated_id_++; +} + Result> DataTypeJsonParser::ParseType( const std::string& name, const rapidjson::Value& type_json_value) { + FieldIdAssigner field_id_assigner; + return ParseType(name, type_json_value, &field_id_assigner); +} + +Result> DataTypeJsonParser::ParseType( + const std::string& name, const rapidjson::Value& type_json_value, + FieldIdAssigner* field_id_assigner) { if (type_json_value.IsString()) { return ParseAtomicTypeField(name, type_json_value); } else if (type_json_value.IsObject()) { - return ParseComplexTypeField(name, type_json_value); + return ParseComplexTypeField(name, type_json_value, field_id_assigner); } return Status::Invalid("cannot parse data type"); } +Result DataTypeJsonParser::ParseDataField(const rapidjson::Value& field_json_value) { + return ParseDataField(field_json_value, /*field_id_assigner=*/nullptr); +} + +Result DataTypeJsonParser::ParseDataField(const rapidjson::Value& field_json_value, + FieldIdAssigner* field_id_assigner) { + if (!field_json_value.IsObject()) { + return Status::Invalid("data field must be an object"); + } + std::optional explicit_id; + if (field_json_value.HasMember("id") && !field_json_value["id"].IsNull()) { + if (!field_json_value["id"].IsInt()) { + return Status::Invalid("value of key 'id' must be int"); + } + explicit_id = field_json_value["id"].GetInt(); + } + int32_t id = -1; + if (field_id_assigner != nullptr) { + PAIMON_ASSIGN_OR_RAISE(id, field_id_assigner->Assign(explicit_id)); + } else if (explicit_id.has_value()) { + id = explicit_id.value(); + } else { + return Status::Invalid("key 'id' must exist"); + } + if (!field_json_value.HasMember("name")) { + return Status::Invalid("key 'name' must exist"); + } + const auto& name_value = field_json_value["name"]; + if (!name_value.IsString()) { + return Status::Invalid("value of key 'name' must be string"); + } + std::string name(name_value.GetString(), name_value.GetStringLength()); + if (!field_json_value.HasMember("type")) { + return Status::Invalid("key 'type' must exist"); + } + Result> field_result = + ParseType(name, field_json_value["type"], field_id_assigner); + if (!field_result.ok()) { + return Status::Invalid( + fmt::format("parse data type failed, error msg: {}", field_result.status().ToString())); + } + std::optional description; + if (field_json_value.HasMember("description") && !field_json_value["description"].IsNull()) { + const auto& description_value = field_json_value["description"]; + if (!description_value.IsString()) { + return Status::Invalid("value of key 'description' must be string"); + } + description = + std::string(description_value.GetString(), description_value.GetStringLength()); + } + return DataField(id, field_result.value(), description); +} + Result> DataTypeJsonParser::ParseAtomicTypeField( const std::string& name, const rapidjson::Value& type_json_value) { bool nullable = true; @@ -674,7 +749,8 @@ Result> DataTypeJsonParser::ParseAtomicTypeField( } Result> DataTypeJsonParser::ParseComplexTypeField( - const std::string& name, const rapidjson::Value& type_json_value) { + const std::string& name, const rapidjson::Value& type_json_value, + FieldIdAssigner* field_id_assigner) { if (!type_json_value.HasMember("type")) { return Status::Invalid("complex data type must have type"); } @@ -686,13 +762,13 @@ Result> DataTypeJsonParser::ParseComplexTypeField( } if (StringUtils::StartsWith(type_str, "ARRAY")) { - return ParseArrayType(name, type_json_value, nullable); + return ParseArrayType(name, type_json_value, nullable, field_id_assigner); } else if (StringUtils::StartsWith(type_str, "VECTOR")) { - return ParseVectorType(name, type_json_value, nullable); + return ParseVectorType(name, type_json_value, nullable, field_id_assigner); } else if (StringUtils::StartsWith(type_str, "MAP")) { - return ParseMapType(name, type_json_value, nullable); + return ParseMapType(name, type_json_value, nullable, field_id_assigner); } else if (StringUtils::StartsWith(type_str, "ROW")) { - return ParseRowType(name, type_json_value, nullable); + return ParseRowType(name, type_json_value, nullable, field_id_assigner); } else if (StringUtils::StartsWith(type_str, "MULTISET")) { return Status::NotImplemented("MULTISET is not supported"); } @@ -701,18 +777,20 @@ Result> DataTypeJsonParser::ParseComplexTypeField( } Result> DataTypeJsonParser::ParseArrayType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable) { + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner) { if (!type_json_value.HasMember("element")) { return Status::Invalid("array data type must have element"); } PAIMON_ASSIGN_OR_RAISE(std::shared_ptr element_field, - ParseType("item", type_json_value["element"])); + ParseType("item", type_json_value["element"], field_id_assigner)); return arrow::field(name, arrow::list(element_field), nullable); } Result> DataTypeJsonParser::ParseVectorType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable) { + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner) { if (!type_json_value.HasMember("element") || !type_json_value.HasMember("length")) { return Status::Invalid("vector data type must have element and length"); } @@ -724,7 +802,7 @@ Result> DataTypeJsonParser::ParseVectorType( return Status::Invalid("Vector length must be between 1 and 2147483647 (both inclusive)"); } PAIMON_ASSIGN_OR_RAISE(std::shared_ptr element_field, - ParseType("item", type_json_value["element"])); + ParseType("item", type_json_value["element"], field_id_assigner)); if (!VectorType::IsValidElementType(element_field->type())) { return Status::Invalid( fmt::format("Invalid element type for vector: {}", element_field->type()->ToString())); @@ -733,13 +811,14 @@ Result> DataTypeJsonParser::ParseVectorType( } Result> DataTypeJsonParser::ParseMapType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable) { + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner) { if (!type_json_value.HasMember("key") || !type_json_value.HasMember("value")) { return Status::Invalid("map data type must have key and value"); } PAIMON_ASSIGN_OR_RAISE(std::shared_ptr key, - ParseType("key", type_json_value["key"])); + ParseType("key", type_json_value["key"], field_id_assigner)); // NOTE: Unlike Java Paimon, this C++ implementation does not support nullable keys in // MapType. This is a limitation of Apache Arrow, which does not allow null keys in its @@ -748,14 +827,24 @@ Result> DataTypeJsonParser::ParseMapType( // interoperating with Java Paimon. key = key->WithNullable(false); PAIMON_ASSIGN_OR_RAISE(std::shared_ptr value, - ParseType("value", type_json_value["value"])); + ParseType("value", type_json_value["value"], field_id_assigner)); return arrow::field(name, std::make_shared(key, value), nullable); } Result> DataTypeJsonParser::ParseRowType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable) { - auto data_fields = - RapidJsonUtil::DeserializeKeyValue>(type_json_value, "fields"); + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner) { + if (!type_json_value.HasMember("fields") || !type_json_value["fields"].IsArray()) { + return Status::Invalid("row data type must have fields"); + } + const auto& field_json_array = type_json_value["fields"].GetArray(); + std::vector data_fields; + data_fields.reserve(field_json_array.Size()); + for (const auto& field_json_value : field_json_array) { + PAIMON_ASSIGN_OR_RAISE(DataField data_field, + ParseDataField(field_json_value, field_id_assigner)); + data_fields.push_back(std::move(data_field)); + } auto struct_type = DataField::ConvertDataFieldsToArrowStructType(data_fields); return arrow::field(name, struct_type, nullable); diff --git a/src/paimon/common/types/data_type_json_parser.h b/src/paimon/common/types/data_type_json_parser.h index 2134236a7..ae302454e 100644 --- a/src/paimon/common/types/data_type_json_parser.h +++ b/src/paimon/common/types/data_type_json_parser.h @@ -19,12 +19,13 @@ #pragma once +#include #include +#include #include #include "arrow/api.h" #include "paimon/common/types/data_field.h" -#include "paimon/common/utils/rapidjson_util.h" #include "paimon/result.h" #include "rapidjson/document.h" @@ -34,7 +35,8 @@ class DataTypeJsonParser { DataTypeJsonParser() = delete; ~DataTypeJsonParser() = delete; - /// Parses a data type from a JSON value and returns an Arrow field representation. + /// Parses JSON into an Arrow field. If all ROW fields omit 'id', assigns IDs in preorder + /// starting at 0. Preserves explicit IDs and rejects partially specified IDs. /// /// @param name The name of the field. /// @param type_json_value The JSON value representing the type. @@ -42,20 +44,49 @@ class DataTypeJsonParser { static Result> ParseType(const std::string& name, const rapidjson::Value& type_json_value); + /// Parses a schema field, requiring explicit IDs on the field and all nested ROW fields. + /// + /// @param field_json_value The JSON value representing the field. + /// @return A Result containing the parsed DataField, or an error status if parsing fails. + static Result ParseDataField(const rapidjson::Value& field_json_value); + private: + /// Shares ID assignment across a type tree and rejects partially specified IDs. + class FieldIdAssigner { + public: + Result Assign(const std::optional& explicit_id); + + private: + int32_t next_generated_id_ = 0; + bool has_explicit_id_ = false; + }; + + /// Reuses the type tree's ID assigner; nullptr requires explicit IDs on all ROW fields. + static Result> ParseType(const std::string& name, + const rapidjson::Value& type_json_value, + FieldIdAssigner* field_id_assigner); + + static Result ParseDataField(const rapidjson::Value& field_json_value, + FieldIdAssigner* field_id_assigner); + static Result> ParseAtomicTypeField( const std::string& name, const rapidjson::Value& type_json_value); static Result> ParseComplexTypeField( - const std::string& name, const rapidjson::Value& type_json_value); + const std::string& name, const rapidjson::Value& type_json_value, + FieldIdAssigner* field_id_assigner); static Result> ParseArrayType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable); + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner); static Result> ParseVectorType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable); + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner); static Result> ParseMapType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable); + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner); static Result> ParseRowType( - const std::string& name, const rapidjson::Value& type_json_value, bool nullable); + const std::string& name, const rapidjson::Value& type_json_value, bool nullable, + FieldIdAssigner* field_id_assigner); }; } // namespace paimon diff --git a/src/paimon/common/types/data_type_json_parser_test.cpp b/src/paimon/common/types/data_type_json_parser_test.cpp index e5dfbc21e..5049afc38 100644 --- a/src/paimon/common/types/data_type_json_parser_test.cpp +++ b/src/paimon/common/types/data_type_json_parser_test.cpp @@ -24,6 +24,7 @@ #include "gtest/gtest.h" #include "paimon/common/data/variant/variant_type_utils.h" +#include "paimon/common/types/data_field.h" #include "paimon/common/utils/checked_cast.h" #include "paimon/common/utils/date_time_utils.h" #include "paimon/status.h" @@ -34,6 +35,15 @@ namespace paimon::test { +namespace { + +void CheckFieldId(const std::shared_ptr& field, int32_t expected_id) { + ASSERT_OK_AND_ASSIGN(DataField data_field, DataField::ConvertArrowFieldToDataField(field)); + ASSERT_EQ(data_field.Id(), expected_id); +} + +} // namespace + TEST(DataTypeJsonParserTest, ParseTypeArrayTypeSuccess) { const std::string name = "array_field"; const char* json = R"({ @@ -127,6 +137,13 @@ TEST(DataTypeJsonParserTest, ParseTypeRowTypeSuccess) { "id" : 4, "name" : "sub4", "type" : "BYTES" + }, { + "id" : 7, + "name" : "sub7", + "type" : { + "type" : "ROW", + "fields" : [ { "id" : 9, "name" : "nested", "type" : "INT" } ] + } }]})"; rapidjson::Document doc; doc.Parse(json); @@ -134,6 +151,116 @@ TEST(DataTypeJsonParserTest, ParseTypeRowTypeSuccess) { ASSERT_OK_AND_ASSIGN(std::shared_ptr field, DataTypeJsonParser::ParseType(name, doc)); ASSERT_NE(field, nullptr); + ASSERT_EQ(field->type()->num_fields(), 3); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(field->type()->field(0), 1)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(field->type()->field(1), 4)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(field->type()->field(2), 7)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(field->type()->field(2)->type()->field(0), 9)); +} + +TEST(DataTypeJsonParserTest, ParseTypeRowTypeGeneratesMissingFieldIds) { + const char* json = R"({ + "type" : "ROW", + "fields" : [ { + "name" : "payload", + "type" : { + "type" : "ROW", + "fields" : [ { + "name" : "age", + "type" : "INT" + }, { + "name" : "profile", + "type" : { + "type" : "ROW", + "fields" : [ { "name" : "city", "type" : "STRING" } ] + } + } ] + } + }, { + "name" : "tags", + "type" : { + "type" : "ARRAY", + "element" : { + "type" : "ROW", + "fields" : [ { "name" : "tag", "type" : "STRING" } ] + } + } + }, { + "name" : "props", + "type" : { + "type" : "MAP", + "key" : { + "type" : "ROW", + "fields" : [ { "name" : "code", "type" : "STRING" } ] + }, + "value" : { + "type" : "ROW", + "fields" : [ { "name" : "weight", "type" : "DOUBLE" } ] + } + } + } ]})"; + rapidjson::Document doc; + doc.Parse(json); + + for (int32_t attempt = 0; attempt < 2; ++attempt) { + SCOPED_TRACE(attempt); + ASSERT_OK_AND_ASSIGN(std::shared_ptr field, + DataTypeJsonParser::ParseType("row_field", doc)); + const std::shared_ptr& row = field->type(); + ASSERT_EQ(row->num_fields(), 3); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(row->field(0), 0)); + const std::shared_ptr& payload = row->field(0)->type(); + ASSERT_EQ(payload->num_fields(), 2); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(payload->field(0), 1)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(payload->field(1), 2)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(payload->field(1)->type()->field(0), 3)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(row->field(1), 4)); + const std::shared_ptr& element = row->field(1)->type()->field(0)->type(); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(element->field(0), 5)); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(row->field(2), 6)); + const std::shared_ptr& entries = row->field(2)->type()->field(0)->type(); + const std::shared_ptr& map_key = entries->field(0)->type(); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(map_key->field(0), 7)); + const std::shared_ptr& map_value = entries->field(1)->type(); + ASSERT_NO_FATAL_FAILURE(CheckFieldId(map_value->field(0), 8)); + } +} + +TEST(DataTypeJsonParserTest, ParseTypeComplexTypeFailure) { + const std::vector> test_cases = { + {R"({"type":"ROW"})", "row data type must have fields"}, + {R"({"type":"ROW","fields":{}})", "row data type must have fields"}, + {R"({"type":"ROW","fields":[null]})", "data field must be an object"}, + {R"({"type":"ROW","fields":[{"name":"a","type":"INT"}, + {"id":1,"name":"b","type":"INT"}]})", + "Partial field id is not allowed."}, + {R"({"type":"ROW","fields":[{"id":0,"name":"a","type":"INT"}, + {"name":"b","type":"INT"}]})", + "Partial field id is not allowed."}, + {R"({"type":"ROW","fields":[{"id":0,"name":"a","type":{ + "type":"ROW","fields":[{"name":"b","type":"INT"}]}}]})", + "Partial field id is not allowed."}, + {R"({"type":"ROW","fields":[{"name":"a","type":{ + "type":"ROW","fields":[{"id":1,"name":"b","type":"INT"}]}}]})", + "Partial field id is not allowed."}, + {R"({"type":"ROW","fields":[{"name":"a","type":{"type":"ARRAY", + "element":{"type":"ROW","fields":[{"id":1,"name":"b","type":"INT"}]}}}]})", + "Partial field id is not allowed."}, + {R"({"type":"MAP", + "key":{"type":"ROW","fields":[{"id":0,"name":"a","type":"INT"}]}, + "value":{"type":"ROW","fields":[{"name":"b","type":"INT"}]}})", + "Partial field id is not allowed."}, + {R"({"type":"MAP", + "key":{"type":"ROW","fields":[{"name":"a","type":"INT"}]}, + "value":{"type":"ROW","fields":[{"id":1,"name":"b","type":"INT"}]}})", + "Partial field id is not allowed."}, + }; + for (const auto& [json, error_msg] : test_cases) { + SCOPED_TRACE(json); + rapidjson::Document doc; + doc.Parse(json); + ASSERT_NOK_WITH_MSG(DataTypeJsonParser::ParseType("row_field", doc), error_msg); + } } TEST(DataTypeJsonParserTest, ParseTypeAtomicTypeSuccess) { diff --git a/src/paimon/format/parquet/variant_parquet_test.cpp b/src/paimon/format/parquet/variant_parquet_test.cpp index 2c1bab1de..94c501f53 100644 --- a/src/paimon/format/parquet/variant_parquet_test.cpp +++ b/src/paimon/format/parquet/variant_parquet_test.cpp @@ -503,6 +503,24 @@ constexpr const char* kNestedShreddingSchema = R"({ } ] })"; +constexpr const char* kNestedShreddingSchemaWithoutIds = R"({ + "type": "ROW", + "fields": [ { + "name": "v", + "type": { + "type": "ROW", + "fields": [ + {"name": "age", "type": "INT"}, + {"name": "addr", "type": { + "type": "ROW", + "fields": [ {"name": "city", "type": "STRING"} ] + }}, + {"name": "tags", "type": {"type": "ARRAY", "element": "STRING"}} + ] + } + } ] +})"; + void CollectFieldIds(const ::parquet::schema::Node& node, const std::string& prefix, std::map* field_ids) { std::string path = prefix.empty() ? node.name() : prefix + "." + node.name(); @@ -648,12 +666,18 @@ TEST_F(VariantParquetTest, ShreddedWriteAndReadRoundTrip) { R"({"age": "not a number", "extra": [1, 2]})", "[\"top level array\"]", }; - for (const std::string mode : {"configured", "per-file", "adaptive"}) { + const std::map> expected_object_ids = { + {"configured", {{"age", 3}, {"addr", 4}, {"city", 5}, {"tags", 6}}}, + {"configured-without-ids", {{"age", 1}, {"addr", 2}, {"city", 3}, {"tags", 4}}}, + {"per-file", {{"age", 1}, {"addr", 0}, {"city", 0}, {"tags", 2}}}, + {"adaptive", {{"age", 1}, {"addr", 0}, {"city", 0}, {"tags", 2}}}}; + for (const auto& [mode, object_ids] : expected_object_ids) { SCOPED_TRACE(mode); - const bool configured = mode == "configured"; + const bool configured = mode == "configured" || mode == "configured-without-ids"; std::map option_map; if (configured) { - option_map[Options::VARIANT_SHREDDING_SCHEMA] = kNestedShreddingSchema; + option_map[Options::VARIANT_SHREDDING_SCHEMA] = + mode == "configured" ? kNestedShreddingSchema : kNestedShreddingSchemaWithoutIds; } else { option_map[Options::VARIANT_INFER_SHREDDING_SCHEMA] = "true"; option_map[Options::VARIANT_SHREDDING_INFERENCE_MODE] = mode; @@ -703,16 +727,16 @@ TEST_F(VariantParquetTest, ShreddedWriteAndReadRoundTrip) { {"v.metadata", 0}, {"v.value", 1}, {"v.typed_value", 2}, - {"v.typed_value.age", configured ? 3 : 1}, + {"v.typed_value.age", object_ids.at("age")}, {"v.typed_value.age.value", 0}, {"v.typed_value.age.typed_value", 1}, - {"v.typed_value.addr", configured ? 4 : 0}, + {"v.typed_value.addr", object_ids.at("addr")}, {"v.typed_value.addr.value", 0}, {"v.typed_value.addr.typed_value", 1}, - {"v.typed_value.addr.typed_value.city", configured ? 5 : 0}, + {"v.typed_value.addr.typed_value.city", object_ids.at("city")}, {"v.typed_value.addr.typed_value.city.value", 0}, {"v.typed_value.addr.typed_value.city.typed_value", 1}, - {"v.typed_value.tags", configured ? 6 : 2}, + {"v.typed_value.tags", object_ids.at("tags")}, {"v.typed_value.tags.value", 0}, {"v.typed_value.tags.typed_value", 1}, {"v.typed_value.tags.typed_value.list", -1},