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

Jump to bottom

Fix querying with a path into nested collections with wildcards #7404

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Fix querying with a path into nested collections with wildcards #7404

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3a5e559
Fix querying with a path into nested collections with wildcards
jedelbo Feb 28, 2024
fb66b64
Fix compilation and update test
jedelbo Mar 4, 2024
483b5dd
Merge branch 'master' into je/query-wildcard
jedelbo Mar 4, 2024
e061380
Do not merge values from different objects into a common list in queries
jedelbo Mar 5, 2024
1057eb6
Update after review
jedelbo Mar 6, 2024
6220240
Update test
jedelbo Mar 7, 2024
5833c2e
Merge branch 'master' into je/query-wildcard
jedelbo Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* None.
* Query lists vs lists if the property to check is a path with wildcards would not give correct result ([#7393](https://github.com/realm/realm-core/issues/7393), since v14.0.0)

### Breaking changes
* None.
Expand Down
41 changes: 24 additions & 17 deletions src/realm/collection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,20 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
{
auto path_size = ctrl.path.size() - index;
PathElement& pe = ctrl.path[index];
bool end_of_path = path_size == 1;

if (end_of_path) {
ctrl.matches.emplace_back();
}

if (val.is_type(type_Dictionary) && (pe.is_key() || pe.is_all())) {
auto ref = val.get_ref();
if (!ref)
return;
Array top(ctrl.alloc);
Array top(*ctrl.alloc);
top.init_from_ref(ref);

BPlusTree<StringData> keys(ctrl.alloc);
BPlusTree<StringData> keys(*ctrl.alloc);
keys.set_parent(&top, 0);
keys.init_from_parent();
size_t start = 0;
Expand All @@ -156,28 +162,29 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
start = keys.find_first(StringData(pe.get_key()));
if (start == realm::not_found) {
if (pe.get_key() == "@keys") {
ctrl.from_list = true;
ctrl.path_only_unary_keys = false;
REALM_ASSERT(end_of_path);
keys.for_all([&](const auto& k) {
ctrl.matches.insert(k);
ctrl.matches.back().push_back(k);
});
}
else {
ctrl.matches.insert(Mixed());
ctrl.matches.back().push_back(Mixed());
}
return;
}
finish = start + 1;
}
BPlusTree<Mixed> values(ctrl.alloc);
BPlusTree<Mixed> values(*ctrl.alloc);
values.set_parent(&top, 1);
values.init_from_parent();
for (; start < finish; start++) {
val = values.get(start);
if (path_size > 1) {
Collection::get_any(ctrl, val, index + 1);
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
ctrl.matches.insert(val);
Collection::get_any(ctrl, val, index + 1);
}
}
}
Expand All @@ -186,7 +193,7 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
auto ref = val.get_ref();
if (!ref)
return;
BPlusTree<Mixed> list(ctrl.alloc);
BPlusTree<Mixed> list(*ctrl.alloc);
list.init_from_ref(ref);
if (size_t sz = list.size()) {
size_t start = 0;
Expand All @@ -202,11 +209,11 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
}
for (; start < finish; start++) {
val = list.get(start);
if (path_size > 1) {
Collection::get_any(ctrl, val, index + 1);
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
ctrl.matches.insert(val);
Collection::get_any(ctrl, val, index + 1);
}
}
}
Expand All @@ -217,15 +224,15 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
auto col = obj.get_table()->get_column_key(pe.get_key());
if (col) {
val = obj.get_any(col);
if (path_size > 1) {
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
if (val.is_type(type_Link)) {
val = ObjLink(obj.get_target_table(col)->get_key(), val.get<ObjKey>());
}
Collection::get_any(ctrl, val, index + 1);
}
else {
ctrl.matches.insert(val);
}
}
}
}
Expand Down
17 changes: 5 additions & 12 deletions src/realm/collection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,11 @@ class Collection {
virtual StablePath get_stable_path() const = 0;

struct QueryCtrlBlock {
QueryCtrlBlock(Path& p, const Table& table, bool is_from_list)
: path(p)
, from_list(is_from_list)
, alloc(table.get_alloc())
, group(table.get_parent_group())
{
}
Path& path;
std::set<Mixed> matches;
bool from_list;
Allocator& alloc;
Group* group;
Path path;
std::vector<std::vector<Mixed>> matches;
bool path_only_unary_keys = false; // Not from list
Allocator* alloc = nullptr;
Group* group = nullptr;
};
static void get_any(QueryCtrlBlock&, Mixed, size_t);
};
Expand Down
4 changes: 4 additions & 0 deletions src/realm/obj.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,6 +1979,10 @@ Dictionary Obj::get_dictionary(ColKey col_key) const
Obj& Obj::set_collection(ColKey col_key, CollectionType type)
{
REALM_ASSERT(col_key.get_type() == col_type_Mixed);
if ((col_key.is_dictionary() && type == CollectionType::Dictionary) ||
(col_key.is_list() && type == CollectionType::List)) {
return *this;
}
update_if_needed();
Mixed new_val(0, type);

Expand Down
103 changes: 64 additions & 39 deletions src/realm/query_expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,19 @@ ColumnDictionaryKeys Columns<Dictionary>::keys()

void Columns<Dictionary>::init_path(const PathElement* begin, const PathElement* end)
{
m_path.clear();
m_path_only_unary_keys = true;
m_ctrl.path.clear();
m_ctrl.path_only_unary_keys = true;
while (begin != end) {
if (begin->is_all()) {
m_path_only_unary_keys = false;
m_ctrl.path_only_unary_keys = false;
}
m_path.emplace_back(std::move(*begin));
m_ctrl.path.emplace_back(std::move(*begin));
++begin;
}
std::move(begin, end, std::back_inserter(m_path));
if (m_path.empty()) {
m_path_only_unary_keys = false;
m_path.push_back(PathElement::AllTag());
std::move(begin, end, std::back_inserter(m_ctrl.path));
if (m_ctrl.path.empty()) {
m_ctrl.path_only_unary_keys = false;
m_ctrl.path.push_back(PathElement::AllTag());
}
}

Expand All @@ -298,27 +298,38 @@ void ColumnDictionaryKeys::set_cluster(const Cluster* cluster)
}


void ColumnDictionaryKeys::evaluate(size_t index, ValueBase& destination)
void ColumnDictionaryKeys::reset_path(size_t index)
{
m_links.clear();
if (m_link_map.has_links()) {
REALM_ASSERT(!m_leaf);
std::vector<ObjKey> links = m_link_map.get_links(index);
auto sz = links.size();
m_links = m_link_map.get_links(index);
}
m_link_map_index = 0;
}

// Here we don't really know how many values to expect
std::vector<Mixed> values;
for (size_t t = 0; t < sz; t++) {
const Obj obj = m_link_map.get_target_table()->get_object(links[t]);
bool ColumnDictionaryKeys::more() const
{
return m_link_map_index < m_links.size();
}

void ColumnDictionaryKeys::evaluate(size_t index, ValueBase& destination)
{
if (m_link_map.has_links()) {
if (more()) {
const Obj obj = m_link_map.get_target_table()->get_object(m_links[m_link_map_index++]);
auto dict = obj.get_dictionary(m_column_key);
destination.init(true, dict.size());
// Insert all values
dict.for_all_keys<StringData>([&values](const Mixed& value) {
values.emplace_back(value);
size_t n = 0;
dict.for_all_keys<StringData>([&](const Mixed& value) {
destination.set(n, value);
n++;
});
}

// Copy values over
destination.init(true, values.size());
destination.set(values.begin(), values.end());
else {
destination.init(true, 0);
}
}
else {
// Not a link column
Expand Down Expand Up @@ -350,11 +361,17 @@ class DictionarySize : public Columns<Dictionary> {
: Columns<Dictionary>(other)
{
}
void reset_path(size_t) override {}
bool more() const override
{
return false;
}

void evaluate(size_t index, ValueBase& destination) override
{
Allocator& alloc = this->m_link_map.get_target_table()->get_alloc();
Value<int64_t> list_refs;
this->get_lists(index, list_refs, 1);
this->get_lists(index, list_refs);
Comment on lines -357 to +356
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd that we never requested more than one list anywhere. There should be room for the multi-value row-by-row comparison in cases like this. Eg. for queries such as dictionary.@size() == my_int_property we can grab 8 lists at a time, and set destination.init(not_from_list, 8) and set all the values to the sizes of the lists. It is a niche optimization that only works for only_unary_links that end in a collection count. Not blocking because we didn't have that optimized before, just observing that I think this is the case that the extra parameter was intended for.

destination.init(list_refs.m_from_list, list_refs.size());
for (size_t i = 0; i < list_refs.size(); i++) {
ref_type ref = to_ref(list_refs[i].get_int());
Expand All @@ -375,37 +392,50 @@ SizeOperator<int64_t> Columns<Dictionary>::size()
return SizeOperator<int64_t>(std::move(ptr));
}

void Columns<Dictionary>::evaluate(size_t index, ValueBase& destination)
void Columns<Dictionary>::reset_path(size_t index)
{
Collection::QueryCtrlBlock ctrl(m_path, *m_link_map.get_target_table(), !m_path_only_unary_keys);

m_ctrl.matches.clear();
if (links_exist()) {
REALM_ASSERT(!m_leaf);
std::vector<ObjKey> links = m_link_map.get_links(index);
auto sz = links.size();
if (!m_link_map.only_unary_links())
ctrl.from_list = true;
m_ctrl.path_only_unary_keys = false;

for (size_t t = 0; t < sz; t++) {
const Obj obj = m_link_map.get_target_table()->get_object(links[t]);
auto val = obj.get_any(m_column_key);
if (!val.is_null()) {
Collection::get_any(ctrl, val, 0);
Collection::get_any(m_ctrl, val, 0);
}
}
}
else {
// Not a link column
REALM_ASSERT(m_leaf);
if (ref_type ref = to_ref(m_leaf->get(index))) {
Collection::get_any(ctrl, {ref, CollectionType::Dictionary}, 0);
Collection::get_any(m_ctrl, {ref, CollectionType::Dictionary}, 0);
}
}
if (m_ctrl.matches.empty()) {
// Make sure we at lease have one empty result
m_ctrl.matches.emplace_back();
}
m_destination_index = 0;
}

bool Columns<Dictionary>::more() const
{
return m_destination_index < m_ctrl.matches.size();
}

void Columns<Dictionary>::evaluate(size_t, ValueBase& destination)
{
// Copy values over
auto sz = ctrl.matches.size();
destination.init(ctrl.from_list || sz == 0, sz);
destination.set(ctrl.matches.begin(), ctrl.matches.end());
auto& matches = m_ctrl.matches[m_destination_index++];
auto sz = matches.size();
destination.init(!m_ctrl.path_only_unary_keys || sz == 0, sz);
destination.set(matches.begin(), matches.end());
}


Expand Down Expand Up @@ -440,7 +470,7 @@ void ColumnListBase::set_cluster(const Cluster* cluster)
}
}

void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination, size_t nb_elements)
void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination)
{
if (m_link_map.has_links()) {
std::vector<ObjKey> links = m_link_map.get_links(index);
Expand All @@ -465,13 +495,8 @@ void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination, size_t
}
}
else {
size_t rows = std::min(m_leaf->size() - index, nb_elements);

destination.init(false, rows);

for (size_t t = 0; t < rows; t++) {
destination.set(t, m_leaf->get(index + t));
}
destination.init(false, 1);
destination.set(0, m_leaf->get(index));
}
}

Expand Down