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.

Sign up for GitHub

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

Jump to bottom

Basic support for nested collections #6452

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Basic support for nested collections #6452

merged 2 commits into from
Mar 31, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Mar 30, 2023

What, How & Why?

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Mar 30, 2023
@jedelbo jedelbo force-pushed the je/nested-collections branch 3 times, most recently from 72dd2cd to 720e62d Compare March 30, 2023 10:00
@jedelbo jedelbo marked this pull request as ready for review March 30, 2023 12:12
nicola-cab
nicola-cab approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

LGTM.

nicola-cab
nicola-cab reviewed Mar 30, 2023
View reviewed changes
test/test_list.cpp
test_lists_numeric_agg<Decimal128>(test_context, sg, type_Decimal, Decimal128(realm::null()), true);
}

TEST(List_NestedListColumns)
Copy link
Member

Choose a reason for hiding this comment

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

Just a general comment we are missing some testing around links since you have added support for them compared to the original implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR soes not include support for links

ironage
ironage reviewed Mar 30, 2023
View reviewed changes
src/realm/collection_list.cpp
, m_col_key(col_key)
, m_top(*m_alloc)
, m_refs(*m_alloc)
, m_key_type(coll_type == CollectionType::List ? type_Int : type_String)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect there to be some way to specify the key type of dictionaries, but the API here doesn't allow any control over that. Will we not support intermediate Dictionaries of Int key types by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we will support dictionaries with other key types than strings in the foreseeable future. The only reason we support integers in our Dictionary implementation was just to indicate how that would work.

src/realm/table.cpp Outdated
break;
}
if (collection_types.size() > 1) {
// FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed? I think you handle the spec below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a leftover from an earlier stage

src/realm/column_type.hpp Outdated
@@ -24,6 +24,8 @@

namespace realm {

enum class CollectionType { List, Set, Dictionary };
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is part of the file format in the spec, can you assign explicit values and add a comment that changing these will mean a file format breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/realm/collection_list.hpp
CollectionListPtr get_collection_list(size_t ndx) const;

void remove(size_t ndx); // TODO
void remove(StringData key); // TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

todo

Copy link
Member

Choose a reason for hiding this comment

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

This stuff should have been implemented already in je/list-list. Probably we can remove this code if we intend to merge it into a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes in a subsequent PR

src/realm/spec.cpp Outdated
@@ -323,6 +331,15 @@ void Spec::erase_column(size_t column_ndx)
REALM_ASSERT(column_ndx < m_types.size());

if (ColumnType(int(m_types.get(column_ndx))) != col_type_BackLink) {
if (auto ref = m_top.get_as_ref(3)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could be nice to have a named constant for position 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/test_list.cpp
auto list = obj.get_collection_list(list_col1);
CHECK(list->is_empty());
auto collection = list->insert_collection(0);
dynamic_cast<Lst<Int>*>(collection.get())->add(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

SDKs will probably want a more typed API than using dynamic casts. At least that has been the traditional approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, lets see. Anyway that would be implemented in the ObjectStore interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway - I added a couple of functions. Will actually make some testing easier to write.

ironage
ironage reviewed Mar 30, 2023
View reviewed changes
test/test_list.cpp
CHECK_EQUAL(table->get_nesting_levels(list_col2), 2);
}

TEST(List_NestedList)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also use some testing around removing nested collections and removing columns of nested collections.

Copy link
Member

Choose a reason for hiding this comment

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

More testing has already been added around this are, things have been cherry-picked in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added removing of the column. Removing nested collections will come later.

@nicola-cab nicola-cab self-requested a review March 30, 2023 18:26
nicola-cab
nicola-cab approved these changes Mar 31, 2023
View reviewed changes
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

It is still missing some of the original code, LGTM.

@jedelbo jedelbo changed the title Je/nested collections Basic support for nested collections Mar 31, 2023
@jedelbo jedelbo merged commit 817b67a into next-major Mar 31, 2023
@jedelbo jedelbo deleted the je/nested-collections branch March 31, 2023 12:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ironage ironage ironage left review comments

@nicola-cab nicola-cab nicola-cab approved these changes

Assignees

@jedelbo jedelbo

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jedelbo @nicola-cab @ironage