Conversation
jedelbo
force-pushed
the
jf/mql
branch
4 times, most recently
from
3b74cb4 to
36dab22
Compare
Pull Request Test Coverage Report for Build github_pull_request_285284
💛 - Coveralls |
ironage
left a comment
ironage
left a comment
There was a problem hiding this comment.
Nice work!
A couple suggestions to round things out, but as a prototype, I think it is fine to add this in given that it is an additive change. 👍
| case QueryLogicalOperators::$not: | ||
| node = m_parse_nodes.create<NotNode>(get_query_node(static_cast<bson::BsonDocument>(value))); | ||
| break; | ||
| case QueryLogicalOperators::$nor: |
There was a problem hiding this comment.
Should probably throw something here if it isn't supported?
src/realm/parser/query_bson.cpp
Outdated
| REALM_ASSERT(value.type() == bson::Bson::Type::Array); | ||
| std::vector<QueryNode*> nodes = get_query_nodes(static_cast<bson::BsonArray>(value)); | ||
| REALM_ASSERT(nodes.size() >= 2); | ||
| node = m_parse_nodes.create<AndNode>(nodes[0], nodes[1]); |
There was a problem hiding this comment.
LogicalNode::children is public, so it should be fairly straight forward to append any additional conditions beyond the first two. Same with the handling of "$or" below.
src/realm/parser/query_bson.cpp
Outdated
| if (logical_operators.count(key)) { | ||
| switch (logical_operators[key]) { | ||
| case QueryLogicalOperators::$and: { | ||
| REALM_ASSERT(value.type() == bson::Bson::Type::Array); |
There was a problem hiding this comment.
I think these types of conditions should be throwing "invalid query" or something similar rather than asserting. But not blocking.
| )""""); | ||
| CHECK(object_results.size() == 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you test a few non-happy paths to make sure that a reasonably worded syntax error is thrown?
Suggestions:
{ "non-existing-property": "something" }{ str_col: { $regex: "(?i)a(?-i)cme" } }(non-supported query operator "$regex"){ "mal-formed-bson" : }
|
Let's not merge this yet. |
|
@jsflax What is going to happen with this PR? |
|
Unclear right now– we need to road map this before moving forward with the actual feature. However, if this is adding enough additional useful stuff, I'd be fine removing the MQL features from the header and then consider merging the rest. |
|
@jsflax you mean removing the changes to results.hpp? I am not sure what the risk of keeping it would be. I would prefer that we can keep the test. |
3 participants
No description provided.