Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allocate arguments for lists #6674

Merged
merged 13 commits into from
May 29, 2023
Merged

Allocate arguments for lists #6674

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0af448a
Test manipulation of query argument string buffer after parsing query.
rorbech May 12, 2023
503d898
Merge branch 'master' into cr/query-in-string-list
rorbech May 24, 2023
f97dd54
Update `string in list` test to use results
rorbech May 24, 2023
7aa936f
allocate memory for list arguments in queries
nicola-cab May 25, 2023
9b72ab3
changelog + account for null mixed
nicola-cab May 25, 2023
5804377
lint build fixes
nicola-cab May 25, 2023
956ae1b
code review suggestions
nicola-cab May 26, 2023
3c718c0
changelog
nicola-cab May 26, 2023
ffbdfa4
fix changelog conflicts
nicola-cab May 26, 2023
9b7e632
code review
nicola-cab May 26, 2023
a304617
fix warning
nicola-cab May 26, 2023
6961fcf
Merge branch 'master' of github.com:realm/realm-core into nc/allocate…
nicola-cab May 26, 2023
6a0f9bf
fix changelog
nicola-cab May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* Access token refresh for websockets was not updating the location metadata ([#6630](https://github.com/realm/realm-core/issues/6630), since v13.9.3)
* Fix several UBSan failures which did not appear to result in functional bugs ([#6649](https://github.com/realm/realm-core/pull/6649)).
* Fix an out-of-bounds read in sectioned results when sectioned are removed by modifying all objects in that section to no longer appear in that section ([#6649](https://github.com/realm/realm-core/pull/6649), since v13.12.0)
* Fix allocate arguments for list in queries. ([#6674](https://github.com/realm/realm-core/pull/6674), since v12.5.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest noting that this only affects the query parser.


### Breaking changes
* None.
Expand Down
221 changes: 138 additions & 83 deletions src/realm/parser/driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,12 +1246,17 @@ std::unique_ptr<Subexpr> ConstantNode::visit(ParserDriver* drv, DataType hint)
}
else if (drv->m_args.is_argument_list(arg_no)) {
std::vector<Mixed> mixed_list = drv->m_args.list_for_argument(arg_no);
auto args_in_list = clone_list_of_args(mixed_list);
std::unique_ptr<Value<Mixed>> values = std::make_unique<Value<Mixed>>();
constexpr bool is_list = true;
values->init(is_list, mixed_list.size());
size_t ndx = 0;
for (auto& val : mixed_list) {
values->set(ndx++, val);
for (auto& expr : args_in_list) {
if (expr)
values->set(ndx++, expr->get_mixed());
}
if (!args_in_list.empty()) {
values->set_list_args(args_in_list);
}
if (m_comp_type) {
values->set_comparison_type(*m_comp_type);
Expand All @@ -1262,87 +1267,7 @@ std::unique_ptr<Subexpr> ConstantNode::visit(ParserDriver* drv, DataType hint)
auto type = drv->m_args.type_for_argument(arg_no);
explain_value_message =
util::format("argument %1 of type '%2'", explain_value_message, get_data_type_name(type));
switch (type) {
case type_Int:
ret = std::make_unique<Value<int64_t>>(drv->m_args.long_for_argument(arg_no));
break;
case type_String:
ret = std::make_unique<ConstantStringValue>(drv->m_args.string_for_argument(arg_no));
break;
case type_Binary:
ret = std::make_unique<ConstantBinaryValue>(drv->m_args.binary_for_argument(arg_no));
break;
case type_Bool:
ret = std::make_unique<Value<Bool>>(drv->m_args.bool_for_argument(arg_no));
break;
case type_Float:
ret = std::make_unique<Value<float>>(drv->m_args.float_for_argument(arg_no));
break;
case type_Double: {
// In realm-js all number type arguments are returned as double. If we don't cast to the
// expected type, we would in many cases miss the option to use the optimized query node
// instead of the general Compare class.
double val = drv->m_args.double_for_argument(arg_no);
switch (hint) {
case type_Int:
case type_Bool: {
int64_t int_val = int64_t(val);
// Only return an integer if it precisely represents val
if (double(int_val) == val)
ret = std::make_unique<Value<int64_t>>(int_val);
else
ret = std::make_unique<Value<double>>(val);
break;
}
case type_Float:
ret = std::make_unique<Value<float>>(float(val));
break;
default:
ret = std::make_unique<Value<double>>(val);
break;
}
break;
}
case type_Timestamp: {
try {
ret = std::make_unique<Value<Timestamp>>(drv->m_args.timestamp_for_argument(arg_no));
}
catch (const std::exception&) {
ret = std::make_unique<Value<ObjectId>>(drv->m_args.objectid_for_argument(arg_no));
}
break;
}
case type_ObjectId: {
try {
ret = std::make_unique<Value<ObjectId>>(drv->m_args.objectid_for_argument(arg_no));
}
catch (const std::exception&) {
ret = std::make_unique<Value<Timestamp>>(drv->m_args.timestamp_for_argument(arg_no));
}
break;
}
case type_Decimal:
ret = std::make_unique<Value<Decimal128>>(drv->m_args.decimal128_for_argument(arg_no));
break;
case type_UUID:
ret = std::make_unique<Value<UUID>>(drv->m_args.uuid_for_argument(arg_no));
break;
case type_Link:
ret = std::make_unique<Value<ObjKey>>(drv->m_args.object_index_for_argument(arg_no));
break;
case type_TypedLink:
if (hint == type_Mixed || hint == type_Link || hint == type_TypedLink) {
ret = std::make_unique<Value<ObjLink>>(drv->m_args.objlink_for_argument(arg_no));
break;
}
explain_value_message =
util::format("%1 which links to %2", explain_value_message,
print_pretty_objlink(drv->m_args.objlink_for_argument(arg_no),
drv->m_base_table->get_parent_group()));
break;
default:
break;
}
ret = clone_arg(drv, type, arg_no, hint, explain_value_message);
}
break;
}
Expand All @@ -1355,6 +1280,136 @@ std::unique_ptr<Subexpr> ConstantNode::visit(ParserDriver* drv, DataType hint)
return ret;
}

std::vector<std::shared_ptr<Subexpr>> ConstantNode::clone_list_of_args(std::vector<Mixed>& mixed_args)
{
std::vector<std::shared_ptr<Subexpr>> args_in_list;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, I'd recommend building up a ConstantMixedList. It should take care of owning the data. That will simplify a lot of code at the call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly,

 void ConstantMixedList ::set(size_t n, Mixed val)
    {
        Value<Mixed>::set(n, val);
        (*this)[n].use_buffer(m_buffer[n]);
    }

which copies the buffer only for string and binary.

switch (get_type()) {
        case type_String:
            buf = std::string(string_val);
            string_val = StringData(buf);
            break;
        case type_Binary:
            buf = std::string(binary_val);
            binary_val = BinaryData(buf);
            break;
        default:
            break;
    }

It seems to me that these are the only 2 types for which we need to deep copy stuff. So it should be fine. Thanks for the suggestion.

args_in_list.reserve(mixed_args.size());
std::shared_ptr<Subexpr> ret;

for (const auto& mixed : mixed_args) {
if (!mixed.is_null()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Null isn't being handled correctly here (it is inserting the last moved value of ret into the list).

const auto& type = mixed.get_type();

switch (type) {
case type_Int:
ret = std::make_unique<Value<int64_t>>(mixed.get_int());
break;
case type_String:
ret = std::make_unique<ConstantStringValue>(mixed.get_string());
break;
case type_Binary:
ret = std::make_unique<ConstantBinaryValue>(mixed.get_binary());
break;
case type_Bool:
ret = std::make_unique<Value<Bool>>(mixed.get_bool());
break;
case type_Float:
ret = std::make_unique<Value<float>>(mixed.get_float());
break;
case type_Double:
ret = std::make_unique<Value<double>>(mixed.get_double());
break;
case type_Timestamp:
ret = std::make_unique<Value<Timestamp>>(mixed.get_timestamp());
break;
case type_ObjectId:
ret = std::make_unique<Value<ObjectId>>(mixed.get_object_id());
break;
case type_Decimal:
ret = std::make_unique<Value<Decimal128>>(mixed.get_decimal());
break;
case type_UUID:
ret = std::make_unique<Value<UUID>>(mixed.get_uuid());
break;
case type_Link:
ret = std::make_unique<Value<ObjKey>>(mixed.get_link().get_obj_key());
break;
case type_TypedLink:
ret = std::make_unique<Value<ObjLink>>(mixed.get_link());
break;
default:
break;
}
}
args_in_list.push_back(std::move(ret));
}

return args_in_list;
}

std::unique_ptr<Subexpr> ConstantNode::clone_arg(ParserDriver* drv, DataType type, size_t arg_no, DataType hint,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

std::string& err)
{
switch (type) {
case type_Int:
return std::make_unique<Value<int64_t>>(drv->m_args.long_for_argument(arg_no));
case type_String:
return std::make_unique<ConstantStringValue>(drv->m_args.string_for_argument(arg_no));
case type_Binary:
return std::make_unique<ConstantBinaryValue>(drv->m_args.binary_for_argument(arg_no));
case type_Bool:
return std::make_unique<Value<Bool>>(drv->m_args.bool_for_argument(arg_no));
case type_Float:
return std::make_unique<Value<float>>(drv->m_args.float_for_argument(arg_no));
case type_Double: {
// In realm-js all number type arguments are returned as double. If we don't cast to the
// expected type, we would in many cases miss the option to use the optimized query node
// instead of the general Compare class.
double val = drv->m_args.double_for_argument(arg_no);
switch (hint) {
case type_Int:
case type_Bool: {
int64_t int_val = int64_t(val);
// Only return an integer if it precisely represents val
if (double(int_val) == val)
return std::make_unique<Value<int64_t>>(int_val);
else
return std::make_unique<Value<double>>(val);
}
case type_Float:
return std::make_unique<Value<float>>(float(val));
default:
return std::make_unique<Value<double>>(val);
}
break;
}
case type_Timestamp: {
try {
return std::make_unique<Value<Timestamp>>(drv->m_args.timestamp_for_argument(arg_no));
}
catch (const std::exception&) {
return std::make_unique<Value<ObjectId>>(drv->m_args.objectid_for_argument(arg_no));
}
}
case type_ObjectId: {
try {
return std::make_unique<Value<ObjectId>>(drv->m_args.objectid_for_argument(arg_no));
}
catch (const std::exception&) {
return std::make_unique<Value<Timestamp>>(drv->m_args.timestamp_for_argument(arg_no));
}
break;
}
case type_Decimal:
return std::make_unique<Value<Decimal128>>(drv->m_args.decimal128_for_argument(arg_no));
case type_UUID:
return std::make_unique<Value<UUID>>(drv->m_args.uuid_for_argument(arg_no));
case type_Link:
return std::make_unique<Value<ObjKey>>(drv->m_args.object_index_for_argument(arg_no));
case type_TypedLink:
if (hint == type_Mixed || hint == type_Link || hint == type_TypedLink) {
return std::make_unique<Value<ObjLink>>(drv->m_args.objlink_for_argument(arg_no));
}
err = util::format("%1 which links to %2", err,
print_pretty_objlink(drv->m_args.objlink_for_argument(arg_no),
drv->m_base_table->get_parent_group()));
break;
default:
break;
}
return nullptr;
}

#if REALM_ENABLE_GEOSPATIAL
GeospatialNode::GeospatialNode(GeospatialNode::Box, GeoPoint& p1, GeoPoint& p2)
: m_geo{Geospatial{GeoBox{p1, p2}}}
Expand Down
3 changes: 3 additions & 0 deletions src/realm/parser/driver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ class ConstantNode : public ValueNode {
{
target_table = table_name.substr(1, table_name.size() - 2);
}

std::vector<std::shared_ptr<Subexpr>> clone_list_of_args(std::vector<Mixed>&);
std::unique_ptr<Subexpr> clone_arg(ParserDriver*, DataType, size_t, DataType, std::string&);
std::unique_ptr<Subexpr> visit(ParserDriver*, DataType) override;
util::Optional<ExpressionComparisonType> m_comp_type;
std::string target_table;
Expand Down
6 changes: 6 additions & 0 deletions src/realm/query_expression.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,6 +1316,11 @@ class Value : public ValueBase, public Subexpr2<T> {
m_comparison_type = type;
}

void set_list_args(std::vector<std::shared_ptr<Subexpr>>& args_list)
{
m_args_list.swap(args_list);
}

Mixed get_mixed() const override
{
return get(0);
Expand All @@ -1333,6 +1338,7 @@ class Value : public ValueBase, public Subexpr2<T> {

protected:
util::Optional<ExpressionComparisonType> m_comparison_type;
std::vector<std::shared_ptr<Subexpr>> m_args_list;
};

class ConstantMixedValue : public Value<Mixed> {
Expand Down
35 changes: 35 additions & 0 deletions test/object-store/c_api/c_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2373,6 +2373,41 @@ TEST_CASE("C API", "[c_api]") {
CHECK_ERR_CAT(RLM_ERR_INDEX_OUT_OF_BOUNDS, (RLM_ERR_CAT_INVALID_ARG | RLM_ERR_CAT_LOGIC));
}

SECTION("string in list") {
char foo[] = "foo";
realm_value_t str = rlm_str_val(foo);
realm_value_t list_arg[2] = {str, rlm_str_val("bar")};

write([&]() {
CHECK(realm_set_value(obj1.get(), foo_properties["string"], rlm_str_val("foo"), false));
});

static const size_t num_args = 1;
realm_query_arg_t args[num_args] = {realm_query_arg_t{1, false, &str}};
realm_query_arg_t* arg_list_simple = &args[0];

realm_query_arg_t args_in_list[num_args] = {realm_query_arg_t{2, true, &list_arg[0]}};
realm_query_arg_t* arg_list = &args_in_list[0];

auto q_string_single_param =
cptr_checked(realm_query_parse(realm, class_foo.key, "string == $0", num_args, arg_list_simple));
auto q_string_in_list =
cptr_checked(realm_query_parse(realm, class_foo.key, "string IN $0", num_args, arg_list));

// changing the value for one of the parameters passed should not change the result of the query.
// essentially we must assure that core is copying all the arguments passed inside the list (like for
// normal query arguments), and after realm_query_parse completes any modification of the memory that
// was used to store the parameter does not impact in any way core.
char* s = foo;
s[0] = 'a';
size_t count, count_list;

CHECK(checked(realm_query_count(q_string_single_param.get(), &count)));
CHECK(1 == count);
CHECK(checked(realm_query_count(q_string_in_list.get(), &count_list)));
CHECK(1 == count_list);
}

SECTION("decimal NaN") {
realm_value_t decimal = rlm_decimal_nan();

Expand Down