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.

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

Fix query on size of a single link over links #6918

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Fix query on size of a single link over links #6918

merged 2 commits into from
Aug 24, 2023

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Aug 23, 2023

What, How & Why?

Fixes #6915

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

I have a couple of questions, the logic is not totally clear to me...

CHANGELOG.md Outdated
@@ -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.
* Crash when querying the size of a Object property in a linked/linked to object ([#6915](https://github.com/realm/realm-core/issues/6915), since v13.17.2)
Copy link
Member

Choose a reason for hiding this comment

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

What linked/linked stands for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is not entirely readable. The query should be on the size of an Object property on an object that is linked from the base object. The link can either be forward or backward. I will try to rephrase

@@ -519,11 +519,16 @@ void LinkCount::evaluate(size_t index, ValueBase& destination)
const Obj obj = m_link_map.get_target_table()->get_object(links[i]);
auto val = obj._get<int64_t>(m_column_key.get_index());
size_t s;
if (val & 1) {
if (m_column_key.get_type() == col_type_Link && !m_column_key.is_collection()) {
Copy link
Member

Choose a reason for hiding this comment

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

Do I read this correctly? You are trying to avoid to deal with collections of links? Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We must handle single link columns separately. That was the problem.

// It is a single link column
s = (val == 0) ? 0 : 1;
}
else if (val & 1) {
Copy link
Member

Choose a reason for hiding this comment

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

How can you be sure that, at this point, we are dealing with a simple backlink column that points to one value? I guess what I am trying to say, is that... the logic behind val == 0 vs val == 1 is a little bit obscure, does val represents a simple boolean flag?....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a bit obscure. The only case where we can have a tagged value here is when we are dealing with a backlink column with a single link. All other cases we have a ref to bplustree of links.

Copy link
Member

Choose a reason for hiding this comment

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

OK maybe we can use a comment here. Describing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually a comment

@nicola-cab nicola-cab self-requested a review August 23, 2023 12:48
// It is a single link column
s = (val == 0) ? 0 : 1;
}
else if (val & 1) {
Copy link
Member

Choose a reason for hiding this comment

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

OK maybe we can use a comment here. Describing this.

@jedelbo jedelbo merged commit 060c0a3 into master Aug 24, 2023
@jedelbo jedelbo deleted the je/RCORE-1751 branch August 24, 2023 07:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when evaluating linking objects query
2 participants