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

[hashtable feature] Convert SET datatype to use hashtable instead of dict #1176

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

[hashtable feature] Convert SET datatype to use hashtable instead of dict #1176

wants to merge 1 commit into from
+330 −375

Conversation

SoftlyRaining
Copy link
Contributor

@SoftlyRaining SoftlyRaining commented Oct 16, 2024 •
edited
Loading

This PR follows #1186.

A fairly straightforward conversion, though I had to do a lot of debugging and fixing along the way.

@codecov Codecov
Copy link

codecov bot commented Oct 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.25322% with 53 lines in your changes missing coverage. Please review.

Project coverage is 70.76%. Comparing base (5f7fe9e) to head (22317e7).
Report is 4 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 0.00% 25 Missing ⚠️
src/debug.c 0.00% 14 Missing ⚠️
src/defrag.c 68.18% 7 Missing ⚠️
src/rdb.c 85.71% 3 Missing ⚠️
src/db.c 92.85% 2 Missing ⚠️
src/t_set.c 97.29% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1176      +/-   ##
============================================
- Coverage     70.90%   70.76%   -0.14%     
============================================
  Files           119      119              
  Lines         64631    64692      +61     
============================================
- Hits          45828    45782      -46     
- Misses        18803    18910     +107
Files with missing lines Coverage Δ
src/hashtable.c 75.88% <100.00%> (+1.67%) ⬆️
src/lazyfree.c 86.11% <100.00%> (ø)
src/object.c 82.13% <100.00%> (+1.46%) ⬆️
src/server.c 87.46% <100.00%> (ø)
src/server.h 100.00% <ø> (ø)
src/t_zset.c 95.66% <100.00%> (-0.01%) ⬇️
src/zmalloc.c 82.74% <100.00%> (+0.13%) ⬆️
src/db.c 89.55% <92.85%> (+0.04%) ⬆️
src/t_set.c 97.61% <97.29%> (+0.10%) ⬆️
src/rdb.c 76.19% <85.71%> (-0.27%) ⬇️
... and 3 more

... and 9 files with indirect coverage changes

zuiderkwast
zuiderkwast reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Partial review. I'll look more later.

tests/unit/type/set.tcl Show resolved Hide resolved
src/hashset.c Outdated Show resolved Hide resolved
src/hashset.c Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

Sorry for force-pushing the hashset branch again, to fix a DCO issue.

Can you rebase and force-push again? (I guess it's better than merge when we have a DCO issue following us.)

@zuiderkwast zuiderkwast linked an issue Oct 22, 2024 that may be closed by this pull request
Use new hashtable for sets #1094
Open
zuiderkwast
zuiderkwast reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

This looks really good. Just a few nits.

src/db.c Outdated Show resolved Hide resolved
src/db.c Outdated Show resolved Hide resolved
src/hashset.c Outdated Show resolved Hide resolved
src/t_set.c Outdated Show resolved Hide resolved
src/t_zset.c Outdated Show resolved Hide resolved
tests/unit/type/set.tcl Show resolved Hide resolved
@zuiderkwast zuiderkwast force-pushed the hashset branch 9 times, most recently from 63f785b to 8aa2646 Compare November 20, 2024 19:55
@SoftlyRaining
Copy link
Contributor Author

Thanks Viktor for taking care of the rebase - your changes look good to me. :)

@zuiderkwast
Copy link
Contributor

You're welcome. :) The main thing I don't like right now is OBJ_ENCODING_HT for dict vs OBJ_ENCODING_HASHTABLE for hashtable. Maybe we should rename the former to OBJ_ENCODING_DICT and the latter to OBJ_ENCODING_HT instead?

@SoftlyRaining
Copy link
Contributor Author

That's a fair point, but OBJECT_ENCODING_HT will be removed completely when all data types have been converted to hashset, so it is only an issue short term, I think. I don't want to touch extra files just to rename it temporarily - I feel like that might make the PR more confusing and harder to review. :-/

@zuiderkwast zuiderkwast force-pushed the hashset branch 6 times, most recently from c957864 to d6da54f Compare December 2, 2024 21:41
@zuiderkwast zuiderkwast force-pushed the hashset branch 2 times, most recently from dfbea9d to 3ed37ea Compare December 6, 2024 11:14
@zuiderkwast
Copy link
Contributor

Are you updating this? It becomes very hard to rebase it if you wait this long between each rebase.

@zuiderkwast zuiderkwast changed the title [hashset feature] Convert SET datatype to use hashset instead of dict [hashtable feature] Convert SET datatype to use hashtable instead of dict Dec 6, 2024
@SoftlyRaining
Copy link
Contributor Author

SoftlyRaining commented Dec 7, 2024 via email

@zuiderkwast zuiderkwast force-pushed the hashset branch 2 times, most recently from d090f8e to 3559ba8 Compare December 10, 2024 12:55
@zuiderkwast zuiderkwast changed the base branch from hashset to unstable December 10, 2024 20:34
@zuiderkwast
Copy link
Contributor

I rebased it (started from unstable and applied commit with cherry-pick) but then I forgot git cherry-pick --continue after solving the merge conflict. When I pushed, I just pushed unstable, causing the diff to become empty and then the PR closed. Now I can't push to your branch to fix it. 😢

@SoftlyRaining SoftlyRaining reopened this Dec 11, 2024
@SoftlyRaining
Copy link
Contributor Author

I got the PR fixed and back up - ready to review and merge! Thanks @zuiderkwast for your work rebasing this again. 😊

zuiderkwast
zuiderkwast reviewed Dec 11, 2024
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Nits.

Changes to the merged code after code review include avoiding the cast to void **. (Related to the strict aliasing rule or just nicer code?) We should do the corresponding changes here.

When I rebased, I just made it compile. I didn't update anything else, like hs variable names.

src/db.c Outdated Show resolved Hide resolved
src/db.c Outdated Show resolved Hide resolved
src/debug.c Outdated Show resolved Hide resolved
src/debug.c Outdated Show resolved Hide resolved
src/debug.c Outdated Show resolved Hide resolved
src/t_set.c Outdated Show resolved Hide resolved
src/t_set.c Outdated Show resolved Hide resolved
src/t_set.c Outdated Show resolved Hide resolved
src/t_set.c Outdated Show resolved Hide resolved
src/t_zset.c Outdated Show resolved Hide resolved
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Dec 11, 2024
@SoftlyRaining SoftlyRaining force-pushed the set-datatype branch 2 times, most recently from 1138fac to ded690a Compare December 13, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuiderkwast zuiderkwast zuiderkwast left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use new hashtable for sets
2 participants
@SoftlyRaining @zuiderkwast