allow RQL geo args to be strings #6934
Conversation
src/realm/parser/driver.cpp
Outdated
| throw InvalidQueryError(util::format( | ||
| "Invalid syntax in serialized geospatial object at argument %1: '%2'", arg_no, doctored_err)); | ||
| } | ||
| if (GeoWithinNode* node = dynamic_cast<GeoWithinNode*>(sub_driver.result)) { |
There was a problem hiding this comment.
Why these checks and why not static_cast? If parsing is successful we should be good here, no?
There was a problem hiding this comment.
I like the dynamic cast because if the code structure changes in the future (such that parsing is successful but something other than a GeoWithinNode is produced) and the maintainer forgets to update this code path then the queries fail in a controlled manner rather than aborting the program.
There was a problem hiding this comment.
You could use the usual trick by adding a dynamic_cast in an assert statement afterwards.
src/realm/parser/driver.cpp
Outdated
| @@ -797,12 +797,49 @@ Query GeoWithinNode::visit(ParserDriver* drv) | |||
| else { | |||
| size_t arg_no = size_t(strtol(argument.substr(1).c_str(), nullptr, 10)); | |||
| auto right_type = drv->m_args.is_argument_null(arg_no) ? DataType(-1) : drv->m_args.type_for_argument(arg_no); | |||
| if (right_type != type_Geospatial) { | |||
|
|
|||
| Geospatial geo; | |||
There was a problem hiding this comment.
Hides member variable geo. Confusing. Perhaps the member should be geo_node or geospatial,
There was a problem hiding this comment.
Good eye, that is confusing. Updated.
src/realm/parser/driver.cpp
Outdated
| @@ -797,12 +797,49 @@ Query GeoWithinNode::visit(ParserDriver* drv) | |||
| else { | |||
There was a problem hiding this comment.
Why else here? There is a return above
There was a problem hiding this comment.
True. I restructured this to simplify the structure.
|
Test failures are unrelated. |
The SDKs who use the CAPI do not have support for passing geospatial objects as arguments to the query parser. It is not trivial to enable that and so that work was postponed to geospatial phase 2 which has since been deprioritized. This PR enables this by allowing SDKs to pass the geo object as a serialized string instead which is then parsed by core and the geo object reconstructed. In an ideal world, this problem would be solved by the binding generator, but given our timelines this should work as a temporary solution.
Also see this comment in the scope as to why a proper solution in the CAPI support was not implemented.
☑️ ToDos