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

Optimize MixedNode<Equal> unindexed #6506

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Optimize MixedNode<Equal> unindexed #6506

merged 1 commit into from
Apr 20, 2023

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Apr 16, 2023 

Req runs:    5  QueryEqual<mixed><NonNullable><NonIndexed> (MemOnly, EncryptionOff):     min 102.53ms (-36.21%)           max 103.34ms (-35.80%)           med 102.86ms (-36.04%)           avg 102.86ms (-36.05%)           stddev   343us (+235.82%)

☑️ ToDos

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

@cla-bot cla-bot bot added the cla: yes label Apr 16, 2023
@ironage ironage requested a review from jedelbo April 16, 2023 17:51
@ironage ironage requested a review from kiburtse April 16, 2023 17:51
kiburtse
kiburtse reviewed Apr 17, 2023
View reviewed changes
Copy link
Contributor

@kiburtse kiburtse left a comment

Choose a reason for hiding this comment

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

What's up with CI? Otherwise lgtm

src/realm/array_mixed.cpp
@@ -218,7 +218,7 @@ size_t ArrayMixed::find_first(Mixed value, size_t begin, size_t end) const noexc
if (end == realm::npos)
end = size();
for (size_t i = begin; i < end; i++) {
if (this->get_type(i) == type && get(i) == value) {
if (Mixed::data_types_are_comparable(this->get_type(i), type) && get(i) == value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it changes which values get compared now - it also deserves changelog entry, no?

Also, it's fine to change it, 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.

ArrayMixed::find_first was unused before this. When I added it in I found some failing tests, so I think there is no change log needed as there is no net new behaviour.

@ironage
Copy link
Contributor Author

ironage commented Apr 17, 2023

I opened #6508 to track the CI failure.

jedelbo
jedelbo approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

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

Awesome

@ironage ironage merged commit 5b0ded5 into master Apr 20, 2023
@ironage ironage deleted the js/query-mixed-equal branch April 20, 2023 08:36
@Omrisnyk Omrisnyk mentioned this pull request Nov 28, 2023
Open
@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.
Reviewers

@kiburtse kiburtse kiburtse approved these changes

@jedelbo jedelbo jedelbo approved these changes

Assignees

@ironage ironage

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ironage @jedelbo @kiburtse