Query serialisation part 3 #2989

ironage · 2018-01-30T20:25:28Z

This adds support for subquery expressions and backlinks to the query serialiser and parser.
It also sets up the architecture for key path aliasing which could be useful for querying over named backlinks or when bindings allow referencing properties by other user defined names.

This is part of #2943

realm-ci · 2018-01-30T21:23:30Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/1/Performance_Report

astigsen · 2018-01-30T21:29:52Z

Does this also cover the simplified shortcut syntax (IN, ANY, ALL...), or does it only support full subqueries (SUBQUERY())?

ironage · 2018-01-30T21:43:02Z

Currently only the full subquery syntax is supported here SUBQUERY(...).@count as this is the only type of subquery expression that any of the bindings currently support. But I think some of those shortcuts could be built on top of this work.

bdash · 2018-01-30T21:54:50Z

Serialization of Cocoa queries that use ANY is already supported because it's implicit in core's query engine for any query that traverses a to-many relationship. It's simply not present from the serialized form. It doesn't look like the parser supports parsing it either, but I'm not sure how much that matters.

No bindings support the equivalent of ALL at the moment. It could be supported by converting it to a SUBQUERY, but it'd be more efficient if it were implemented directly in the query engine.

IN isn't related to subquery support at all, AFAICT.

astigsen · 2018-01-30T22:04:45Z

IN isn't related to subquery support at all, AFAICT.

x IN listproperty should be equivalent to ANY listproperty = x.

ironage · 2018-01-30T22:09:43Z

For JS queries we could potentially support these shortcuts without native core query support by getting the parser to make the following mappings:

ANY items.price > 10 or 10 IN items.price becomes SUBQUERY(items, $x, $x.price > 10).@count > 0
ALL items.price > 10 becomes SUBQUERY(items, $x, $x.price > 10).@count == items.@count

astigsen · 2018-01-30T22:15:24Z

Yes, I think it is worth supporting them all, even if the initial mapping means that the performance is less than what we could get with a native implementation.

bdash · 2018-01-30T22:17:08Z

x IN listproperty should be equivalent to ANY listproperty = x.

Ah, yes. But again, serializing ANY listproperty = x is already supported.

Please also keep in mind that adding support for parsing additional query features is outside of the scope of the current query serialization work. It needs to be evaluated and prioritized against other work.

codecov-io · 2018-01-30T23:50:17Z

Codecov Report

Merging #2989 into master will increase coverage by 0.05%.
The diff coverage is 95.66%.

@@            Coverage Diff             @@
##           master    #2989      +/-   ##
==========================================
+ Coverage   93.17%   93.22%   +0.05%     
==========================================
  Files         266      270       +4     
  Lines       77914    78627     +713     
==========================================
+ Hits        72594    73302     +708     
- Misses       5320     5325       +5

Impacted Files	Coverage Δ
src/realm/parser/expression_container.hpp	`100% <ø> (ø)`	⬆️
src/realm/query.hpp	`100% <ø> (ø)`	⬆️
src/realm/table.hpp	`97.4% <ø> (+0.01%)`	⬆️
src/realm/parser/parser.hpp	`100% <ø> (ø)`	⬆️
src/realm/parser/parser_utils.hpp	`83.33% <ø> (ø)`	⬆️
src/realm/table.cpp	`90.83% <100%> (+0.03%)`	⬆️
src/realm/parser/property_expression.hpp	`100% <100%> (ø)`	⬆️
src/realm/parser/keypath_mapping.hpp	`100% <100%> (ø)`
src/realm/parser/parser.cpp	`94.68% <100%> (+0.93%)`	⬆️
src/realm/parser/property_expression.cpp	`100% <100%> (ø)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 170f778...bb1cf77. Read the comment docs.

realm-ci · 2018-01-30T23:50:34Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/2/Performance_Report

realm-ci · 2018-02-03T05:50:41Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/5/Performance_Report

realm-ci · 2018-02-04T07:59:40Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/6/Performance_Report

realm-ci · 2018-02-06T10:30:16Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/7/Performance_Report

rrrlasse · 2018-02-06T16:03:44Z

src/realm/util/serializer.cpp

+        std::string guess = guess_prefix + add_char;
+        bool found_duplicate = false;
+        for (size_t i = 0; i < subquery_prefix_list.size(); ++i) {
+            if (guess.compare(subquery_prefix_list[i]) == 0) {


I think guess == subquery_prefix_list[i] is more well known and thus easier to read (I had to look-up how compare() worked to check if there was any special reason why it was called)

rrrlasse · 2018-02-06T16:06:03Z

src/realm/util/serializer.cpp

+        }
+    };
+
+    while (true) {


Missing assert or other handling if the column is never found (should be there even if it by design can't happen)

By design it will loop endlessly until a unique name is found. I assume there must be a finite number of column names to search through and a finite number of subquery nestings with different variable names. In practice though it'll find a match on the first pass. Would you suggest putting an upper limit on the number of loop iterations we allow?

oh okay, I misread the code and thought it would loop until it had somehow found a specific column :) no worries then

rrrlasse · 2018-02-06T16:09:33Z

src/realm/util/serializer.cpp

+    char add_char = start_char;
+
+    auto next_guess = [&]() {
+        add_char = (((add_char + 1) - 'a') % 26) + 'a';


% ('z' - 'a' + 1) is easier to read I think

rrrlasse · 2018-02-06T16:12:27Z

src/realm/parser/subquery_expression.hpp

+template <typename RetType>
+struct SubqueryGetter<RetType,
+typename std::enable_if_t<
+std::is_same<RetType, Int>::value ||


we have realm::is_any :)

rrrlasse · 2018-02-06T16:32:30Z

src/realm/parser/collection_operator_expression.hpp

+            } else {
+                list_property_name = pre_link_table->get_column_name(pe.get_dest_ndx());
+            }
+            precondition(pe.get_dest_type() == type_LinkList || pe.dest_type_is_backlink(),


this precondition macro is defined in a .hpp file (parser_utils.hpp), so it must be prefixed with REALM_ in order not to pollute users´ name spaces

rrrlasse · 2018-02-06T16:37:49Z

src/realm/parser/keypath_mapping.cpp

+
+// This may be premature optimisation, but it'll be super fast and it doesn't
+// bother dragging in anything locale specific for case insensitive comparisons.
+bool is_backlinks_prefix(std::string s) {


If it's super fast, maybe also pass a reference instead of a value :D

rrrlasse · 2018-02-06T16:42:34Z

src/realm/parser/keypath_mapping.hpp

+    static Table* table_getter(TableRef table, const std::vector<KeyPathElement>& links);
+protected:
+    bool m_allow_backlinks;
+    std::map<std::pair<ConstTableRef, std::string>, std::string> m_mapping;


an unordered_map might be alot faster (and has same API)

rrrlasse · 2018-02-06T16:48:40Z

src/realm/parser/keypath_mapping.cpp

+
+void KeyPathMapping::add_mapping(ConstTableRef table, std::string name, std::string alias)
+{
+    m_mapping[{table, name}] = alias;


error handling if the table already exists?

realm-ci · 2018-02-06T19:37:30Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/8/Performance_Report

realm-ci · 2018-02-09T03:21:10Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/9/Performance_Report

realm-ci · 2018-02-10T02:31:36Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/10/Performance_Report

realm-ci · 2018-02-13T21:11:26Z

Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2989/11/Performance_Report

James Stone added 2 commits January 25, 2018 15:48

Serialisation support for subqueries

7fb99e0

Parser support for subquery expressions

81d0d15

ironage added the T-Enhancement label Jan 30, 2018

ironage self-assigned this Jan 30, 2018

ironage mentioned this pull request Jan 30, 2018

Serialisation of subqueries #2941

Closed

Fix gcc issues

54c07f6

James Stone added 3 commits January 31, 2018 15:33

Serialise backlink columns

12a4a8f

Allow ConstTableRef when applying a parsed ordering

ebb1620

Parser support for querying over backlinks

45797e2

ironage mentioned this pull request Feb 3, 2018

Support serialisation of backlinks #2939

Closed

Optionally restrict parsing backlink queries

8a12585

ironage mentioned this pull request Feb 3, 2018

Allow rejection of custom rules in deserialisation #2942

Closed

ironage requested review from bdash, rrrlasse and finnschiermer February 3, 2018 04:32

James Stone added 2 commits February 2, 2018 20:33

Merge branch 'master' into js/query_serialisation_2

decf1a2

Changelog update

6ff3864

ironage changed the base branch from js/query_serialization to master February 3, 2018 04:50

rrrlasse suggested changes Feb 6, 2018

View reviewed changes

rrrlasse reviewed Feb 6, 2018

View reviewed changes

James Stone added 2 commits February 6, 2018 10:23

Add a realm prefix to precondition

4999d59

Review feedback changes

7ed53b7

James Stone added 3 commits February 8, 2018 17:19

Parser support for ANY/SOME/ALL/NONE

6d769eb

Parser support for operator 'IN'

d707e60

Add parser improvements to changelog

bde4a13

Fix a stack use after free issue in the parser, args now required

2dac2f0

rrrlasse approved these changes Feb 13, 2018

View reviewed changes

Review update

bb1cf77

ironage mentioned this pull request Feb 13, 2018

Support all Predicate features in queries realm/realm-js#805

Open

7 tasks

ironage mentioned this pull request Feb 14, 2018

Preliminary work for queries over named backlinks realm/realm-js#1660

Merged

2 tasks

finnschiermer approved these changes Feb 14, 2018

View reviewed changes

ironage merged commit 905345f into master Feb 14, 2018

ironage deleted the js/query_serialisation_2 branch February 14, 2018 17:50

ironage mentioned this pull request Mar 6, 2018

Parser improvements #2978

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query serialisation part 3 #2989

Query serialisation part 3 #2989

ironage commented Jan 30, 2018 •

edited

Loading

realm-ci commented Jan 30, 2018

astigsen commented Jan 30, 2018

ironage commented Jan 30, 2018

bdash commented Jan 30, 2018

astigsen commented Jan 30, 2018

ironage commented Jan 30, 2018

astigsen commented Jan 30, 2018

bdash commented Jan 30, 2018

codecov-io commented Jan 30, 2018 •

edited

Loading

realm-ci commented Jan 30, 2018

realm-ci commented Feb 3, 2018

realm-ci commented Feb 4, 2018

realm-ci commented Feb 6, 2018

rrrlasse Feb 6, 2018 •

edited

Loading

rrrlasse Feb 6, 2018

ironage Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

rrrlasse Feb 6, 2018

realm-ci commented Feb 6, 2018

realm-ci commented Feb 9, 2018

realm-ci commented Feb 10, 2018

realm-ci commented Feb 13, 2018

Query serialisation part 3 #2989

Query serialisation part 3 #2989

Conversation

ironage commented Jan 30, 2018 • edited Loading

realm-ci commented Jan 30, 2018

astigsen commented Jan 30, 2018

ironage commented Jan 30, 2018

bdash commented Jan 30, 2018

astigsen commented Jan 30, 2018

ironage commented Jan 30, 2018

astigsen commented Jan 30, 2018

bdash commented Jan 30, 2018

codecov-io commented Jan 30, 2018 • edited Loading

Codecov Report

realm-ci commented Jan 30, 2018

realm-ci commented Feb 3, 2018

realm-ci commented Feb 4, 2018

realm-ci commented Feb 6, 2018

rrrlasse Feb 6, 2018 • edited Loading

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

realm-ci commented Feb 6, 2018

realm-ci commented Feb 9, 2018

realm-ci commented Feb 10, 2018

realm-ci commented Feb 13, 2018

ironage commented Jan 30, 2018 •

edited

Loading

codecov-io commented Jan 30, 2018 •

edited

Loading

rrrlasse Feb 6, 2018 •

edited

Loading