Use Columns<Link> when property is Dictionary of links #6705

jedelbo · 2023-06-08T14:56:15Z

If a Dictionary property has links as value type, we can use Columns<Link> to handle the links instead of the basic Columns<Dictionary>. This has the effect that when we compare with a single value, we will optimize to use LinksToNode. So we need to make LinksToNode handle the Dictionary case.

When we compare with a list of links, we must ensure that the list is converted to a list of ObjKeys - which is the type that Column<Link> evaluates to.

Fixes #6688

ironage

Could this change be applied to master instead of next-major?

ironage · 2023-06-09T01:25:03Z

src/realm/parser/driver.cpp

+    if (col_type == col_type_LinkList) {
+        col_type = col_type_Link;
+    }
+    if (col_type == col_type_Link) {
+        add(col_key);
+        return create_subexpr<Link>(col_key);
+    }


Suggested change

if (col_type == col_type_LinkList) {

col_type = col_type_Link;

}

if (col_type == col_type_Link) {

add(col_key);

return create_subexpr<Link>(col_key);

}

if (Table::is_link_type(col_type)) {

add(col_key);

return create_subexpr<Link>(col_key);

}

ironage · 2023-06-09T01:54:44Z

src/realm/parser/driver.cpp

+                                             "an argument of type %3",
+                                             link_column->link_map().description(drv->m_serializer_state),
+                                             link_column->link_map().get_target_table()->get_class_name(),
+                                             print_pretty_objlink(value->get(0).get_link(), g)));


Suggested change

print_pretty_objlink(value->get(0).get_link(), g)));

print_pretty_objlink(value->get(i).get_link(), g)));

Thank you for spotting this. I also had this problem when getting the value above. It took me a while to figure out what the problem was.

ironage · 2023-06-09T01:56:30Z

src/realm/parser/driver.cpp

+            for (size_t i = 0; i < sz; i++) {
+                auto val = value->get(i);
+                if (val.is_null()) {
+                    if (sz == 1) {


I think this is missing the logic for when size is not 1. Eg: list IN {NULL, obj('x', 'y')}

I see it like this: this function is only called if list is a list of ObjKey. Such a list can never contain NULL. So adding a NULL to the list to compare against would not make sense.

Doesn't this now apply to a Dictionary of links? In that case there may be null links in the values.

If a Dictionary property has links as value type, we can use Columns<Link> to handle the links instead of the basic Columns<Dictionary>. This has the effect that when we compare with a single value, we will optimize to use LinksToNode. So we need to make LinksToNode handle the Dictionary case. When we compare with a list of links, we must ensure that the list is converted to a list obj ObjKeys - which is the type that Column<Link> evaluates to.

cla-bot · 2023-06-09T13:03:32Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: realm-ci.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

jedelbo · 2023-06-09T13:05:29Z

PR now targeted at master. It turned out that it actually fixes an issue

nicola-cab

A bunch of small comments, but in general it looks good to me.

nicola-cab · 2023-06-09T13:31:01Z

src/realm/parser/driver.cpp

@@ -530,17 +580,12 @@ Query EqualityNode::visit(ParserDriver* drv)
            }
        }
        else if (left_type == type_Link) {
+            REALM_ASSERT(false);


What's the reason for this assert? Maybe something forgotten?

This should have been deleted. A leftover from previous version.

nicola-cab · 2023-06-09T13:36:14Z

src/realm/parser/driver.cpp

+                REALM_ASSERT(dynamic_cast<const Value<ObjKey>*>(right.get()));
+                auto link_values = static_cast<const Value<ObjKey>*>(right.get());
+                // We can use a LinksToNode based query
+                std::vector<ObjKey> values;


I don't know if it makes sense to do it here, but we should know the size of the values to insert in the list of obj keys, so we could do something like: values.reserve(link_values.size());

nicola-cab · 2023-06-09T13:41:27Z

src/realm/query_engine.cpp

-                auto sz = links.size();
-                for (size_t j = 0; j < sz; j++) {
-                    if (links.get(j) != key) {
+    if (m_condition_column_key.is_collection()) {


This code seems the same of the one that starts at line 822, maybe this can be encapsulated into some utility method.

It probably could, but I am not sure the code would be much nicer to read.

ironage · 2023-06-09T21:53:28Z

src/realm/query_engine.hpp

    }

 protected:
    std::vector<ObjKey> m_target_keys;
-    ColumnType m_column_type;


ironage · 2023-06-09T21:54:57Z

src/realm/query_engine.hpp

        m_cluster->init_leaf(this->m_condition_column_key, m_leaf);
    }

    std::string describe(util::serializer::SerialisationState& state) const override
    {
        REALM_ASSERT(m_condition_column_key);
-        if (m_target_keys.size() > 1)
-            throw SerializationError("Serializing a query which links to multiple objects is currently unsupported.");


ironage · 2023-06-09T21:55:39Z

test/test_parser.cpp

+    verify_query(test_context, persons, "[email protected] > 4", 2);
+    verify_query(test_context, persons, "pets.@values == obj('dog', 'pluto')", 2);
+    verify_query(test_context, persons, "pets.@values != obj('dog', 'pluto')", 2);
+    verify_query(test_context, persons, "pets.@values == ANY { obj('dog', 'lady'), obj('dog', 'astro') }", 2);


Is this expected to work? pets.@values == ANY { obj('dog', 'lady'), obj('dog', 'astro'), NULL }

I suppose this is illegal. Fixed

Why should we make it illegal? Null is a valid value in a dictionary.

Damn yes. I was tricked by the question.

* Updated release notes * Update catch2 v3.3.2 (#6605) * Make core infer platform and cpu_arch, while bundle_id must be provided by SDK's (#6612) * platform and cpu_arch are inferred by core, bundle_id must be provided by SDK's * update changelog * Return proper value for X86_64 arch Co-authored-by: Christian Melchior <[email protected]> * Get fine-grained platform for Apple devices * Fix tests * small fixes * fix more tests * Fix mistake in changelog --------- Co-authored-by: Christian Melchior <[email protected]> * use consistent rounding, following SERVER-75392 (#6477) * fix entries that went to the wrong change version (#6632) * Special-case main thread runloop scheduler * Improve SectionedResults performance SectionedResults used a std::map in a few places where the keys are a dense range (i.e. always [0..map.size())) and so they can be std::vector instead. The maps keyed on Mixed are now std::unordered_map. Change notifications now report changes as a `std::vector<IndexSet>` rather than `std::map<size_t, IndexSet>`. This is slower and uses more memory when the only sections that changed are near the end of a SectionedResults with a large number of sections, but is much faster if all sections changed or if the sections which changed are early in the SectionedResults. Change notifications now reuse buffers, which increases persistent memory usage slightly but significant reduces allocations. Change notifications for a single section now only compute the changes for that section rather than computing the full changes and then filtering out the changes for other sections. * use static_assert rather than a old home rolled one * fix warning of redefine of CHECK macro * fix unused function warning * silence warnings in bid128_to_string * Introduce BPlusTree::for_all * Prevent program from crashing when removing backlinks * Fix broken snapshot of collection of objects * Fix importing Results with deleted collection The result should be an empty result, not the whole table. * geospatial validation of polygons (#6607) * geospatial validation of polygons * Loop->Ring, added tests * use std::unique * changelog * Benchmark for full-text search * Allow to filter benchmark and run only list of specified names * Add simple benchmark for fulltext search with index * Filter out unresolved links in Dictionary::get_any() * Add support for early exit in BPlusTree::for_all() * Geospatial feedback (#6645) * verify local results match a server query * disallow geowithin on top level tables * fix geo queries with ANY/ALL/NONE * geospatial validation of points * rename GeoCenterSphere -> GeoCircle * review feedback * better testing and fix any/all/none geospatial * format * Geospatial basic queries benchmarks (#6621) * Add basic benchmarks for Geospatial type and queries * Less copying in GeoWithinCompare * Bring back caching of s2 region into Geospatial * remove transaction overhead from measurements * a couple small optimizations * formatting * simplify geospatial query evaluations * changelog --------- Co-authored-by: James Stone <[email protected]> * Updated baas server tag for CI (#6650) * Prepare release * Updated release notes * Access token refresh for websockets was not updating the location metadata (#6631) * Always refresh metadata on app login * Updated changelog * Always update location when requested; fix c_api test * Update test to properly evaluate websocket redirections; added one more test * Updated changelog and fixed compile warning * Added location checks back to test * added mutex locking around location updated state and reworked requesting location update to use flag * clang format and fix incorrect timeout value * Reworked update location logic a bit and removed unused function * Free mutex before calling completion on early exit in init_app_metadata * maybe fix a race in a test (#6651) * Use std::optional to store cached leaves in query nodes (#6653) Our use of aligned_storage was basically a complicated manual version of this. I was hoping this'd have binary size benefits, but it ended up making the library 100 bytes larger instead. Nonetheless, it greatly simplifies things. * Fix a few UBSan failures hit by tests * Avoid performing unaligned reads in Array::get_chunk() * Fix a lock order inversion in tests (#6666) The cycle was DaemonThread::m_running_on_change_mutex => RealmCoordinator::m_realm_mutex => SyncManager::m_mutex => RealmCoordinator::s_coordinator_mutex => DaemonThread::m_running_on_change_mutex, and it happened due to DaemonThread::remove() being called inside RealmCoordinator::clear_cache() while holding s_coordinator_mutex. Fortunately we don't actually need to be doing that. As the cycle required RealmCoordinator::clear_all_caches(), this was only applicable to tests. * Allow geo coordinate numeric argument substitutions (#6663) * allow geo coordinate numeric argument substitutions * review feedback * explicit cast to address warning * Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() (#6667) * Fix an assertion failure if an async write callback ran during a write transaction (#6661) Between when the callback after acquiring the write lock is scheduled and when it's invoked a synchronous write transaction can be begun, and if it's not ended before the next time the scheduler gets to run, the scheduled callback will be invoked inside the write. When this happens we want to just do nothing. Ending the synchronous write transaction will take care of rescheduling the async write it preempted. * core release 13.13.0 * Updated release notes * Allocate arguments for lists (#6674) * Small documentation and code fixes (#6672) * Fix crash when opening FLX realm after client reset failure (#6671) * Fix crash when opening FLX realm after client reset failure * Update changelog * Don't superceed pending subscriptions in case of a client reset failure * Add test * Changes after code review * Support sorting based on values from a dictionary (#5311) Co-authored-by: Sebastian Valle <[email protected]> Co-authored-by: James Stone <[email protected]> * Filter out external sources from Eclipse (#6682) Indexer has a hard time dealing with Catch2 * Use cross-compilers instead of CentOS image (#6559) * Use cross-compilers instead of CentOS image * changelog * fix bad merge * refactor toolchain files * clarify useToolchain exception circumstances * Remap github URL to ssh to fix BAAS dependency using https:// (#6685) * core v13.14.0 * Updated release notes * Switch to building with Xcode 14 (#6647) * better fix explanation in the changelog for list of args in the query parser (#6692) * Remove constructor for GeoPoint and GeoPolygon (#6679) Co-authored-by: Mathias Stearn <[email protected]> * Fix failing "sync: non-synced metadata table doesn't result in non-additive schema change" tests (#6697) * Reporting correct error message on HTTP errors for Browser target * User/Server API key provider becomes a single 'API key' provider (#6696) * Allow frozen Realms to be opened with additive schema changes (#6693) * allow frozen Realms to be opened with additive schema changes * lint * strengthen tests and comments * Update src/realm/object-store/shared_realm.cpp Co-authored-by: Thomas Goyne <[email protected]> --------- Co-authored-by: Thomas Goyne <[email protected]> * Reverted minimum swift version to fix failing CI tests (#6706) * core release v13.15.0 * Updated release notes * Fix client reset test with invalid query (#6711) * Fix SessionWrapper use-after-free crash when tearing down sessions (#6676) * Changed SessionWrapper pointer to bind_ptr; added session ident history * Fix teardown if client is destroyed before session * Session no longer holds bind_ptr to SessionWrapper; reverted some changes * Fixed return and updated some comments * Don't process errors if session is shutting down * Added extra checks for session state * Updates from review * Updated some finalized checks * Rolled back some changes * Added output to ASSERTS and moved session history to unordered_set * Remove session history entry on normal close * Updated comment in sync tests * Add [baas] and [local] tags to object store sync tests to identify the tests that rely on BAAS or not (#6710) * Use Columns<Link> when property is Dictionary of links (#6705) If a Dictionary property has links as value type, we can use Columns<Link> to handle the links instead of the basic Columns<Dictionary>. This has the effect that when we compare with a single value, we will optimize to use LinksToNode. So we need to make LinksToNode handle the Dictionary case. When we compare with a list of links, we must ensure that the list is converted to a list obj ObjKeys - which is the type that Column<Link> evaluates to. Use LinksToNode for lists in QueryParser * better changelog message for the fix related to queries with list of arguments (#6717) * Fixes for Emscripten target (Passing header from fetch response. Using Config.path for inMemory Realm) (#6716) * Fixes for Emscripten target: Passing header for fetch response. Passing the RealmConfig.path to be used for inMemory Realm, this is needed for registering SyncSession Co-authored-by: Jørgen Edelbo <[email protected]> * release 13.15.1 * Updated spec.yml to remove User & Server prefix from ApiKey credentials --------- Co-authored-by: James Stone <[email protected]> Co-authored-by: realm-ci <[email protected]> Co-authored-by: Kirill Burtsev <[email protected]> Co-authored-by: Daniel Tabacaru <[email protected]> Co-authored-by: Christian Melchior <[email protected]> Co-authored-by: Thomas Goyne <[email protected]> Co-authored-by: Thomas Goyne <[email protected]> Co-authored-by: Jørgen Edelbo <[email protected]> Co-authored-by: Michael Wilkerson-Barker <[email protected]> Co-authored-by: Nicola Cabiddu <[email protected]> Co-authored-by: Sebastian Valle <[email protected]> Co-authored-by: Yavor Georgiev <[email protected]> Co-authored-by: Ferdinando Papale <[email protected]> Co-authored-by: Mathias Stearn <[email protected]> Co-authored-by: Nabil Hachicha <[email protected]> Co-authored-by: Finn Schiermer Andersen <[email protected]>

cla-bot bot added the cla: yes label Jun 8, 2023

github-actions bot assigned jedelbo Jun 8, 2023

jedelbo requested a review from ironage June 8, 2023 14:57

jedelbo changed the title ~~Use Columns<Link> when property is Dictionary if links~~ Use Columns<Link> when property is Dictionary of links Jun 8, 2023

ironage reviewed Jun 9, 2023

View reviewed changes

nicola-cab mentioned this pull request Jun 9, 2023

Query with list of links as argument #6686

Closed

3 tasks

jedelbo added 3 commits June 9, 2023 14:33

Review update

7a33c49

Use LinksToNode for lists in QueryParser

3a1cad3

jedelbo force-pushed the je/column-links branch from 3538c44 to 3a1cad3 Compare June 9, 2023 13:03

cla-bot bot removed the cla: yes label Jun 9, 2023

jedelbo changed the base branch from next-major to master June 9, 2023 13:03

jedelbo requested a review from nicola-cab June 9, 2023 13:04

jedelbo requested a review from ironage June 9, 2023 13:05

nicola-cab requested changes Jun 9, 2023

View reviewed changes

ironage reviewed Jun 9, 2023

View reviewed changes

src/realm/query_engine.hpp

}

protected:

std::vector<ObjKey> m_target_keys;

ColumnType m_column_type;

Copy link

Contributor

ironage Jun 9, 2023

Choose a reason for hiding this comment

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

👍

ironage reviewed Jun 9, 2023

View reviewed changes

Review update 2

54eff5e

cla-bot bot added the cla: yes label Jun 12, 2023

Merge branch 'master' into je/column-links

f9307b1

jedelbo requested review from ironage and nicola-cab June 12, 2023 09:01

nicola-cab approved these changes Jun 12, 2023

View reviewed changes

cmelchior mentioned this pull request Jun 12, 2023

Add support for list query arguments realm/realm-kotlin#1389

Merged

Review update 3

5de3e5b

ironage approved these changes Jun 13, 2023

View reviewed changes

Merge branch 'master' into je/column-links

330189d

jedelbo merged commit d861035 into master Jun 14, 2023

jedelbo deleted the je/column-links branch June 14, 2023 07:26

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Columns<Link> when property is Dictionary of links #6705

Use Columns<Link> when property is Dictionary of links #6705

jedelbo commented Jun 8, 2023 •

edited

Loading

ironage left a comment

ironage Jun 9, 2023

ironage Jun 9, 2023

jedelbo Jun 9, 2023

ironage Jun 9, 2023

jedelbo Jun 9, 2023

ironage Jun 9, 2023

cla-bot bot commented Jun 9, 2023

jedelbo commented Jun 9, 2023

nicola-cab left a comment

nicola-cab Jun 9, 2023

jedelbo Jun 12, 2023

nicola-cab Jun 9, 2023

jedelbo Jun 12, 2023

nicola-cab Jun 9, 2023

jedelbo Jun 12, 2023

ironage Jun 9, 2023

ironage Jun 9, 2023

ironage Jun 9, 2023

jedelbo Jun 12, 2023

ironage Jun 12, 2023

jedelbo Jun 13, 2023

	print_pretty_objlink(value->get(0).get_link(), g)));
	print_pretty_objlink(value->get(i).get_link(), g)));

Use Columns<Link> when property is Dictionary of links #6705

Use Columns<Link> when property is Dictionary of links #6705

Conversation

jedelbo commented Jun 8, 2023 • edited Loading

ironage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Jun 9, 2023

jedelbo commented Jun 9, 2023

nicola-cab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedelbo commented Jun 8, 2023 •

edited

Loading