Je/query index stage 1 #6715

jedelbo · 2023-06-15T06:57:09Z

What, How & Why?

☑️ ToDos

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

nicola-cab · 2023-06-15T09:16:55Z

src/realm/parser/driver.cpp

        }
    }
+    /*


remove... commented code

nicola-cab · 2023-06-15T09:31:27Z

test/test_parser.cpp

@@ -4991,8 +4997,12 @@ TEST(Parser_Dictionary)
    verify_query(test_context, foo, "ALL dict.@type == 'int'", 100);  // all dictionaries have ints
    verify_query(test_context, foo, "NONE dict.@type == 'int'", 0);   // each object has Bar:i
    verify_query(test_context, foo, "ANY dict.@type == 'string'", 0); // no strings present
+    // Dictionaries are not ordered


what's the point of supporting FIRST and LAST for a dictionary?

We are actually testing that we throw in this case

nicola-cab · 2023-06-15T10:07:25Z

src/realm/collection.cpp

+                val = list.get(idx);
+            }
+            if (path_size > 1) {
+                val = Collection::get_any(val, begin + 1, end, alloc);


This can be a problem (probably we are never going to hit it though), in case of nested collections that are too deep, we might run out of stack space.

Any clue when that would be a problem?

It is up to how long is the nested collection... List<List<List<List..... will result in N recursive call proportional to the amount of collections that are nested. Most probably we will never run into this problem in practise, and there are also others part in the code when this can manifest earlier than at this point.

Added a check that nesting level will not exceed 100.

nicola-cab · 2023-06-15T10:08:18Z

src/realm/parser/driver.cpp

@@ -537,7 +537,7 @@ Query EqualityNode::visit(ParserDriver* drv)
    else if (right->has_single_value() && (left_type == right_type || left_type == type_Mixed)) {
        Mixed val = right->get_mixed();
        const ObjPropertyBase* prop = dynamic_cast<const ObjPropertyBase*>(left.get());
-        if (prop && !prop->links_exist()) {
+        if (prop && !prop->links_exist() && !prop->has_path()) {


This is only managing simple values, right? No links?

No links and no path.

nicola-cab · 2023-06-15T10:09:24Z

src/realm/parser/driver.cpp

@@ -879,9 +891,29 @@ std::unique_ptr<Subexpr> PropertyNode::visit(ParserDriver* drv, DataType)
            }
        }
        else {
-            throw InvalidQueryError("Index not supported");
+            // This must be an integer index


Can we assert this somehow?

nicola-cab · 2023-06-15T10:14:48Z

test/test_parser.cpp

@@ -2392,7 +2392,7 @@ TEST_TYPES(Parser_list_of_primitive_types, Prop<Int>, Nullable<Int>, Prop<Bool>,
    auto col_link = t->add_column(*t, "link");

    auto obj1 = t->create_object();
-    std::vector<type> values = gen.values_from_int<type>({0, 9, 4, 2, 7, 4, 1, 8, 11, 3, 4, 5, 22});
+    std::vector<type> values = gen.values_from_int<type>({2, 9, 4, 0, 7, 4, 1, 8, 11, 3, 4, 5, 22});


Why are you swapping 2 and 0? Couldn't you do something like auto value_first = gen.convert_for_test<underlying_type>(0); ... or it is because zero represents null in a mixed?

For some types 0 could result in either 0 or NULL and thus give different results depending on the type. 2 does not have that problem.

ironage · 2023-06-15T17:44:16Z

src/realm/query_expression.hpp

+    friend class Table;
+    friend class LinkChain;
+    bool indexes(const Path& path)
+    {


Suggested change

{

{

REALM_ASSERT(path.size());

ironage · 2023-06-15T17:54:01Z

test/test_parser.cpp

+
+    Obj paul = persons->create_object_with_primary_key("Paul");
+    paul.set(col_self, paul.get_key());
+    auto dict_paul = paul.get_dictionary(col);


How does this work, since this column is not a dictionary? Is this a new nested collection API shortcut that I've missed?

I was actually missing a set_collection call.

I find it a bit strange that the core API allowed that to happen without throwing on a type check.

ironage · 2023-06-15T17:58:52Z

src/realm/query_expression.hpp

+    void evaluate(size_t index, ValueBase& destination) override
+    {
+        ColumnsCollection<Mixed>::evaluate<Mixed>(index, destination);
+        if (m_path.size() > 1) {


is it possible that the path is of length 1? What happens in that case?

Then the base class will use path[0] and no further processing is needed.

ironage · 2023-06-15T18:02:37Z

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### Enhancements
 * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
 * Storage of Decimal128 properties has been optimised so that the individual values will take up 0 bits (if all nulls), 32 bits, 64 bits or 128 bits depending on what is needed. (PR [#6111]https://github.com/realm/realm-core/pull/6111))
+* You can have a collection embedded in any Mixed property (except Set<Mixed>).
+* Quering a specific entry in a list-of-primitives (in particular 'first and 'last') is supported.


ironage · 2023-06-15T18:08:31Z

src/realm/collection.cpp

+        keys.set_parent(&top, 0);
+        keys.init_from_parent();
+        size_t ndx = keys.find_first(StringData(begin->get_key()));
+        if (ndx != realm::not_found) {


I'd like to see some tests where the key is found and there is more levels in the path, but the dictionary key contains a simple primitive value and not another nested level. In that case, we should return Null early and not recurse further (also for lists).

I will add some tests. We will always make a recursive call if the key is found, but that call will return quickly as non of the tests for type will succeed. Do you think this is a problem? Otherwise we would have to do the tests twice.

That seems fine to me 👍

nicola-cab · 2023-06-16T14:29:23Z

It seems we still have some issues with CI.. in particular this seems related: https://spruce.mongodb.com/task/__next_major_windows_64_encryption_core_tests_patch_b47c11c38f447b69508981b3193e695e46b85313_648c4dd457e85ac0f0cb77e6_23_06_16_11_56_05/logs?execution=0

ironage

LGTM when CI is passing.

ironage · 2023-06-16T18:30:25Z

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### Enhancements
 * <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
 * Storage of Decimal128 properties has been optimised so that the individual values will take up 0 bits (if all nulls), 32 bits, 64 bits or 128 bits depending on what is needed. (PR [#6111]https://github.com/realm/realm-core/pull/6111))
+* You can have a collection embedded in any Mixed property (except Set<Mixed>).
+* Quering a specific entry in a list-of-primitives (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269))


Suggested change

* Quering a specific entry in a list-of-primitives (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269))

* Querying a specific entry in a list-of-primitives (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269))

jedelbo added 2 commits June 15, 2023 08:56

Support using index in query on list of primitives

cdf2feb

Support query on nested collections

90fdb13

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

jedelbo requested review from nicola-cab and ironage June 15, 2023 06:57

github-actions bot assigned jedelbo Jun 15, 2023

nicola-cab reviewed Jun 15, 2023

View reviewed changes

ironage reviewed Jun 15, 2023

View reviewed changes

Review update

0826c11

jedelbo requested review from nicola-cab and ironage June 16, 2023 11:56

nicola-cab approved these changes Jun 16, 2023

View reviewed changes

ironage approved these changes Jun 16, 2023

View reviewed changes

Fix test on Windows

a4f9b4c

jedelbo merged commit 26d37e2 into next-major Jun 19, 2023

jedelbo deleted the je/query-index-stage-1 branch June 19, 2023 10:40

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

Je/query index stage 1 #6715

Je/query index stage 1 #6715

jedelbo commented Jun 15, 2023

nicola-cab Jun 15, 2023

nicola-cab Jun 15, 2023

jedelbo Jun 16, 2023

nicola-cab Jun 15, 2023

bmunkholm Jun 15, 2023

nicola-cab Jun 15, 2023

jedelbo Jun 16, 2023

nicola-cab Jun 15, 2023

jedelbo Jun 16, 2023

nicola-cab Jun 15, 2023

jedelbo Jun 16, 2023

nicola-cab Jun 15, 2023

jedelbo Jun 16, 2023

ironage Jun 15, 2023

jedelbo Jun 16, 2023

ironage Jun 15, 2023

jedelbo Jun 16, 2023

ironage Jun 16, 2023

ironage Jun 15, 2023

jedelbo Jun 16, 2023

ironage Jun 15, 2023

jedelbo Jun 16, 2023

ironage Jun 15, 2023

jedelbo Jun 16, 2023 •

edited

Loading

ironage Jun 16, 2023

nicola-cab commented Jun 16, 2023

ironage left a comment

ironage Jun 16, 2023

	* Quering a specific entry in a list-of-primitives (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269))
	* Querying a specific entry in a list-of-primitives (in particular 'first and 'last') is supported. (PR [#4269](https://github.com/realm/realm-core/issues/4269))

Je/query index stage 1 #6715

Je/query index stage 1 #6715

Conversation

jedelbo commented Jun 15, 2023

What, How & Why?

☑️ ToDos

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

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 Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicola-cab commented Jun 16, 2023

ironage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedelbo Jun 16, 2023 •

edited

Loading