Skip to content
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

Schema support for nesting collection #6451

Merged
merged 5 commits into from
Apr 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/realm/object-store/object_schema.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,15 @@ ObjectSchema::ObjectSchema(Group const& group, StringData name, TableKey key)
property.is_fulltext_indexed = table->search_index_type(col_key) == IndexType::Fulltext;
property.column_key = col_key;

// account for nesting collections
const auto nesting_levels = table->get_nesting_levels(col_key);
if (nesting_levels > 0) {
property.nested_types.reserve(nesting_levels);
for (size_t i = 0; i < nesting_levels; ++i) {
property.nested_types.push_back(table->get_nested_column_type(col_key, i));
}
}

if (property.type == PropertyType::Object) {
// set link type for objects and arrays
ConstTableRef linkTable = table->get_link_target(col_key);
Expand Down
1 change: 1 addition & 0 deletions src/realm/object-store/object_schema.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class ObjectSchema {

static PropertyType from_core_type(ColumnType type);
static PropertyType from_core_type(ColKey col);
static PropertyType from_core_type(CollectionType type);

private:
void set_primary_key_property() noexcept;
Expand Down
53 changes: 28 additions & 25 deletions src/realm/object-store/object_store.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,30 @@ DataType to_core_type(PropertyType type)
}
}

std::vector<CollectionType> process_nested_collection(const Property& property)
{
std::vector<CollectionType> collection_types;
// process the list of nested levels
for (const auto& prop_type : property.nested_types) {
collection_types.push_back(prop_type);
}

// check if the final type is itself a collection.
if (is_array(property.type)) {
collection_types.push_back(CollectionType::List);
}
else if (is_set(property.type)) {
collection_types.push_back(CollectionType::Set);
}
else if (is_dictionary(property.type)) {
collection_types.push_back(CollectionType::Dictionary);
}
else if (!collection_types.empty()) {
throw InvalidColumnKey("Not a valid nested collection type");
}
return collection_types;
}

ColKey add_column(Group& group, Table& table, Property const& property)
{
// Cannot directly insert a LinkingObjects column (a computed property).
Expand All @@ -110,37 +134,16 @@ ColKey add_column(Group& group, Table& table, Property const& property)
return col;
}
}
auto collection_types = process_nested_collection(property);
if (property.type == PropertyType::Object) {
auto target_name = ObjectStore::table_name_for_object_type(property.object_type);
TableRef link_table = group.get_table(target_name);
REALM_ASSERT(link_table);
if (is_array(property.type)) {
return table.add_column_list(*link_table, property.name);
}
else if (is_set(property.type)) {
return table.add_column_set(*link_table, property.name);
}
else if (is_dictionary(property.type)) {
return table.add_column_dictionary(*link_table, property.name);
}
else {
return table.add_column(*link_table, property.name);
}
}
else if (is_array(property.type)) {
return table.add_column_list(to_core_type(property.type & ~PropertyType::Flags), property.name,
is_nullable(property.type));
}
else if (is_set(property.type)) {
return table.add_column_set(to_core_type(property.type & ~PropertyType::Flags), property.name,
is_nullable(property.type));
}
else if (is_dictionary(property.type)) {
return table.add_column_dictionary(to_core_type(property.type & ~PropertyType::Flags), property.name,
is_nullable(property.type));
return table.add_column(*link_table, property.name, collection_types);
}
else {
auto key = table.add_column(to_core_type(property.type), property.name, is_nullable(property.type));
auto key = table.add_column(to_core_type(property.type), property.name, is_nullable(property.type),
collection_types);
if (property.requires_index())
table.add_search_index(key);
if (property.requires_fulltext_index())
Expand Down
57 changes: 52 additions & 5 deletions src/realm/object-store/property.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <realm/util/tagged_bool.hpp>

#include <string>

namespace realm {
class BinaryData;
class Decimal128;
Expand Down Expand Up @@ -59,6 +58,7 @@ enum class PropertyType : unsigned short {
Required = 0,
Nullable = 64,
Array = 128,

Set = 256,
Dictionary = 512,

Expand Down Expand Up @@ -96,6 +96,8 @@ struct Property {
IsPrimary is_primary = false;
IsIndexed is_indexed = false;
IsFulltextIndexed is_fulltext_indexed = false;
using NestedTypes = std::vector<CollectionType>;
NestedTypes nested_types;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operator==(Property const& lft, Property const& rgt) needs to be updated to check equality of this new member

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


ColKey column_key;

Expand All @@ -107,9 +109,14 @@ struct Property {
// Text property with fulltext index
Property(std::string name, IsFulltextIndexed indexed, std::string public_name = "");

// Link to another object
Property(std::string name, PropertyType type, std::string object_type, std::string link_origin_property_name = "",
std::string public_name = "");

// Nested collections
Property(std::string name, PropertyType type, const NestedTypes& nested_types, std::string object_type = "",
std::string public_name = "");

Property(Property const&) = default;
Property(Property&&) noexcept = default;
Property& operator=(Property const&) = default;
Expand All @@ -127,6 +134,7 @@ struct Property {

bool type_is_indexable() const noexcept;
bool type_is_nullable() const noexcept;
size_t type_nesting_levels() const noexcept;

std::string type_string() const;
};
Expand Down Expand Up @@ -336,6 +344,18 @@ inline Property::Property(std::string name, PropertyType type, std::string objec
{
}

inline Property::Property(std::string name, PropertyType type, const NestedTypes& nested_types,
std::string target_type, std::string public_name)
: name(std::move(name))
, public_name(std::move(public_name))
, type(type)
, object_type(std::move(target_type))
, nested_types(nested_types)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor appears to be untested, and I don't think it is correct to be setting the nested_types on a linking object property

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can perfectly well have a list of linklist. Anyway - adding a test for it revealed that the two new constructors did not differ on the first 4 parameter types. That had to be fixed - perhaps not in a pretty way, but is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test on list of linklist. But I am still confused about why we would need to set nested_types on a linking object (ie PropertyType::LinkingObjects with a source object_type and link_origin_property_name). I suspect you don't need this new constructor at all. Or maybe I am not understanding how you expect to use it, can you could show how you intend to use it by adding a test?

Copy link
Contributor

@jedelbo jedelbo Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did add a test. But the constructor was intended to be used when defining a property with PropertyType::Object.

Copy link
Contributor

@jedelbo jedelbo Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - now I get it. link_origin_property_name is only needed for PropertyType::LinkingObjects. This can actually be simplified

{
REALM_ASSERT(is_collection(type));
REALM_ASSERT((type == PropertyType::Object) == (object_type.size() != 0));
}

inline bool Property::type_is_indexable() const noexcept
{
return !is_collection(type) &&
Expand All @@ -350,26 +370,52 @@ inline bool Property::type_is_nullable() const noexcept
type != PropertyType::LinkingObjects;
}

inline size_t Property::type_nesting_levels() const noexcept
{
return nested_types.size();
}

inline std::string Property::type_string() const
{
std::string nested;
std::string closing_brackets;
for (auto& type : nested_types) {
switch (type) {
case CollectionType::List:
nested += "array<";
break;
case CollectionType::Dictionary:
nested += "dictionary<string,";
break;
case CollectionType::Set:
REALM_UNREACHABLE();
break;
}
closing_brackets += ">";
}

if (is_array(type)) {
if (type == PropertyType::Object)
return "array<" + object_type + ">";
if (type == PropertyType::LinkingObjects)
return "linking objects<" + object_type + ">";
return std::string("array<") + string_for_property_type(type & ~PropertyType::Flags) + ">";
const auto str = std::string("array<") + string_for_property_type(type & ~PropertyType::Flags) + ">";
return nested + str + closing_brackets;
}
if (is_set(type)) {
REALM_ASSERT(type != PropertyType::LinkingObjects);
if (type == PropertyType::Object)
return "set<" + object_type + ">";
return std::string("set<") + string_for_property_type(type & ~PropertyType::Flags) + ">";
const auto str = std::string("set<") + string_for_property_type(type & ~PropertyType::Flags) + ">";
return nested + str + closing_brackets;
}
if (is_dictionary(type)) {
REALM_ASSERT(type != PropertyType::LinkingObjects);
if (type == PropertyType::Object)
return "dictionary<string, " + object_type + ">";
return std::string("dictionary<string, ") + string_for_property_type(type & ~PropertyType::Flags) + ">";
const auto str =
std::string("dictionary<string, ") + string_for_property_type(type & ~PropertyType::Flags) + ">";
return nested + str + closing_brackets;
}
switch (auto base_type = (type & ~PropertyType::Flags)) {
case PropertyType::Object:
Expand All @@ -388,7 +434,8 @@ inline bool operator==(Property const& lft, Property const& rgt)
return to_underlying(lft.type) == to_underlying(rgt.type) && lft.is_primary == rgt.is_primary &&
lft.requires_index() == rgt.requires_index() &&
lft.requires_fulltext_index() == rgt.requires_fulltext_index() && lft.name == rgt.name &&
lft.object_type == rgt.object_type && lft.link_origin_property_name == rgt.link_origin_property_name;
lft.object_type == rgt.object_type && lft.link_origin_property_name == rgt.link_origin_property_name &&
lft.nested_types == rgt.nested_types;
}
} // namespace realm

Expand Down
4 changes: 4 additions & 0 deletions src/realm/object-store/schema.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ static void compare(ObjectSchema const& existing_schema, ObjectSchema const& tar
changes.emplace_back(schema_change::RemoveProperty{&existing_schema, &current_prop});
continue;
}
if (current_prop.nested_types != target_prop->nested_types) {
changes.emplace_back(schema_change::ChangePropertyType{&existing_schema, &current_prop, target_prop});
continue;
}
if (current_prop.type != target_prop->type || current_prop.object_type != target_prop->object_type ||
is_array(current_prop.type) != is_array(target_prop->type) ||
is_set(current_prop.type) != is_set(target_prop->type) ||
Expand Down
1 change: 1 addition & 0 deletions test/object-store/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ set(SOURCES
thread_safe_reference.cpp
transaction_log_parsing.cpp
uuid.cpp
nested_collections.cpp
c_api/c_api.cpp
c_api/c_api.c

Expand Down
123 changes: 123 additions & 0 deletions test/object-store/migrations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2651,3 +2651,126 @@ TEST_CASE("migration: Manual") {
Catch::Matchers::ContainsSubstring("Property 'object.value' has been removed."));
}
}

TEST_CASE("migration nested collection automatic") {
TestFile config;
config.schema_mode = SchemaMode::Automatic;
auto realm = Realm::get_shared_realm(config);

Schema schema = {
{"nested_object", {{"nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}}}}};
REQUIRE_UPDATE_SUCCEEDS(*realm, schema, 0);

SECTION("Remove nested list") {
Schema new_schema = remove_property(schema, "nested_object", "nested_list");
REQUIRE_UPDATE_SUCCEEDS(*realm, schema, 1);
}

SECTION("Add nested collection") {
Schema new_schema =
add_property(schema, "nested_object",
{"another_nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}});
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}

SECTION("Reorder property") {
Schema new_schema =
add_property(schema, "nested_object",
{"another_nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}});
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
auto& properties = new_schema.find("nested_object")->persisted_properties;
std::swap(properties[0], properties[1]);
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 2);
}

SECTION("Change property type should fail with schema in automatic mode") {
Schema new_schema = set_type(schema, "nested_object", "nested_list", PropertyType::Int);
REQUIRE_UPDATE_FAILS(
*realm, new_schema,
"Property 'nested_object.nested_list' has been changed from 'array<array<int>>' to 'int'.");
}

SECTION("add nested collection to same object") {
Schema new_schema =
add_property(schema, "nested_object",
{"another_nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}});
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}

SECTION("add nested collection with nullable type") {
Schema schema = {
{"nested_object", {{"nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}}}},
{"nested_object2",
{{"nested_list_optional",
PropertyType::Array | PropertyType::Int | PropertyType::Nullable,
{CollectionType::List}}}},
};
REQUIRE_UPDATE_SUCCEEDS(*realm, schema, 1);
}

SECTION("change inner type") {
Schema new_schema = {
{"nested_object",
{{"nested_list",
PropertyType::Array | PropertyType::Mixed | PropertyType::Nullable,
{CollectionType::List}}}},
};
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}

SECTION("change type of nested collection") {
nicola-cab marked this conversation as resolved.
Show resolved Hide resolved
Schema new_schema = {
{"nested_object",
{{"nested_list",
PropertyType::Array | PropertyType::Mixed | PropertyType::Nullable,
{CollectionType::Dictionary}}}},
};
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}
}

TEST_CASE("migration nested collection additive") {

TestFile config;
config.schema_mode = SchemaMode::AdditiveExplicit;
config.schema_subset_mode = SchemaSubsetMode::Strict; // ignore reporting complete schema
auto realm = Realm::get_shared_realm(config);

Schema schema = {
{"nested_object", {{"nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}}}}};
REQUIRE_UPDATE_SUCCEEDS(*realm, schema, 0);

SECTION("Add new column") {
Schema new_schema =
add_property(schema, "nested_object",
{"another_nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::List}});
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}

SECTION("Remove column") {
auto new_schema = remove_property(schema, "nested_object", "nested_list");
REQUIRE_UPDATE_SUCCEEDS(*realm, new_schema, 1);
}

SECTION("Change inner type") {
Schema new_schema = {
{"nested_object",
{{"nested_list",
PropertyType::Array | PropertyType::Mixed | PropertyType::Nullable,
{CollectionType::List}}}},
};
INVALID_SCHEMA_CHANGE(*realm, new_schema,
"Property 'nested_object.nested_list' has been changed from 'array<array<int>>' to "
"'array<array<mixed>>'");
}

SECTION("Change type of nested collection") {
Schema new_schema = {
{"nested_object",
{{"nested_list", PropertyType::Array | PropertyType::Int, {CollectionType::Dictionary}}}},
};
INVALID_SCHEMA_CHANGE(*realm, new_schema,
"Property 'nested_object.nested_list' has been changed from 'array<array<int>>' to "
"'dictionary<string,array<int>>'");
}
}
Loading