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

Improve readability and efficiency for ZUNION operation #1506

Closed
wants to merge 1 commit into from

Conversation

QuChen88
Copy link
Contributor

@QuChen88 QuChen88 commented Jan 4, 2025

Currently zunion operation in t_zset.c is using dictAddRaw() to search for the element in the accumulating dictionary. If the element was not found and got added, it would then set the key to a new key string via dictSetKey(), which is counter intuitive and not very effective.

This PR improves the code readability and effectiveness by using dictFind() to search for the element in the dictionary, and then uses dictAddRaw() and dictSetDoubleVal() to create the element in the set, and avoids the need to call dictSetKey().

Note - this change is consistent with the original implementation of sorted set

de = dictFind(accumulator,zuiSdsFromValue(&zval));
. It was later changed to a "not-so-nice" style in commit 68bf45f

@QuChen88 QuChen88 changed the title Avoid using low level dict APIs for ZUNION operation Improve readability and efficiency for ZUNION operation Jan 4, 2025
@ranshid
Copy link
Member

ranshid commented Jan 5, 2025

Thank you @QuChen88 I think the zset code is about to change a lot following #1427. maybe you want to take a look and propose alternatives there?

@JimB123
Copy link
Contributor

JimB123 commented Jan 6, 2025

I think the reason for the original code is that in a single operation, it searches and adds. With the new code, it will need to perform a hash table lookup for the find, and then another hash table lookup for the add.

@QuChen88
Copy link
Contributor Author

QuChen88 commented Jan 6, 2025

My biggest issue was that the insertion into the dict happened outside of the if (existing) check previously.

The new code in the proposed PR #1427 looks correct to me. It first finds the element in the hashset and then if it is not present will add the element.

@zuiderkwast
Copy link
Contributor

The new code in the proposed PR #1427 looks correct to me. It first finds the element in the hashset and then if it is not present will add the element.

Great, so let's close this PR? #1427 will be merged very soon.

@zuiderkwast zuiderkwast closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants