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

Support HSCAN with NOVALUES argument #2816

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Conversation

gerzse
Copy link
Contributor

@gerzse gerzse commented Apr 4, 2024

Issue #2763

HSCAN has a new argument called NOVALUES. The effect is that only the keys in the hash are returned, without associated values.

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@gerzse gerzse requested review from tishun and atakavci April 4, 2024 12:39
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 88.50575% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 77.68%. Comparing base (43843bf) to head (d98c07a).
Report is 248 commits behind head on main.

Files Patch % Lines
.../api/coroutines/RedisHashCoroutinesCommandsImpl.kt 0.00% 4 Missing ⚠️
src/main/java/io/lettuce/core/ScanIterator.java 76.92% 2 Missing and 1 partial ⚠️
src/main/kotlin/io/lettuce/core/ScanFlow.kt 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2816      +/-   ##
============================================
- Coverage     78.71%   77.68%   -1.03%     
- Complexity     6786     7225     +439     
============================================
  Files           508      537      +29     
  Lines         22765    24462    +1697     
  Branches       2446     2608     +162     
============================================
+ Hits          17919    19004    +1085     
- Misses         3717     4250     +533     
- Partials       1129     1208      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gerzse gerzse requested a review from mp911de April 4, 2024 15:33
Copy link
Collaborator

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

Generally, the functionality is fine. There are some aspects that require further refinement:

  • Our lettercasing uses camel-base based on word boundaries. Since novalues is a word on its own, method names should be hscanNovalues instead of hscanNoValues. We already have some mixture in letter casing. To align with how it should be, please use a lowercase v as in Novalues.
  • Missing @since tags. The next release aims towards 7.0, so please add @since 7.0 to all newly introduced public methods. Adding the tag into the template is typically fine.
  • There are several unrelated changes such as changes to RedisServerAsyncCommands that result from committing all generated interfaces. Please remove these changes from the PR to have a focused pull request.

@gerzse
Copy link
Contributor Author

gerzse commented Apr 5, 2024

Generally, the functionality is fine. There are some aspects that require further refinement:

  • Our lettercasing uses camel-base based on word boundaries. Since novalues is a word on its own, method names should be hscanNovalues instead of hscanNoValues. We already have some mixture in letter casing. To align with how it should be, please use a lowercase v as in Novalues.
  • Missing @since tags. The next release aims towards 7.0, so please add @since 7.0 to all newly introduced public methods. Adding the tag into the template is typically fine.
  • There are several unrelated changes such as changes to RedisServerAsyncCommands that result from committing all generated interfaces. Please remove these changes from the PR to have a focused pull request.

Thanks for the review!

  • I did notice in the codebase the drift in the naming rules. I will switch to the one that you suggested. Also I'll add tags.
  • About the unrelated changes, you're right, they don't belong here. What happened is that I ran the code generator and got many differences, so I though doing something about that. I will open a separate PR only about aligning the templates with the current state of the code, if it's OK.
  • Besides this, @atakavci pointed out that I missed some interfaces that have the hscan method, so I'll try to cover those too, since hscanNovalues is a general alternative to hscan.

Issue redis#2763

HSCAN has a new argument called NOVALUES. The effect is that only the
keys in the hash are returned, without associated values.
@gerzse
Copy link
Contributor Author

gerzse commented Apr 5, 2024

Generally, the functionality is fine. There are some aspects that require further refinement:

  • Our lettercasing uses camel-base based on word boundaries. Since novalues is a word on its own, method names should be hscanNovalues instead of hscanNoValues. We already have some mixture in letter casing. To align with how it should be, please use a lowercase v as in Novalues.
  • Missing @since tags. The next release aims towards 7.0, so please add @since 7.0 to all newly introduced public methods. Adding the tag into the template is typically fine.
  • There are several unrelated changes such as changes to RedisServerAsyncCommands that result from committing all generated interfaces. Please remove these changes from the PR to have a focused pull request.

Thanks for the review!

  • I did notice in the codebase the drift in the naming rules. I will switch to the one that you suggested. Also I'll add tags.
  • About the unrelated changes, you're right, they don't belong here. What happened is that I ran the code generator and got many differences, so I though doing something about that. I will open a separate PR only about aligning the templates with the current state of the code, if it's OK.
  • Besides this, @atakavci pointed out that I missed some interfaces that have the hscan method, so I'll try to cover those too, since hscanNovalues is a general alternative to hscan.

The code generator is a bit of Pandora's box, which I don't want to open. I ran the generator, reverted all the (quite many) unrelated changes, and kept strictly the changes related to HSCAN NOVALUES.

@gerzse gerzse requested a review from mp911de April 5, 2024 14:00
Copy link
Collaborator

@atakavci atakavci left a comment

Choose a reason for hiding this comment

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

looks good!

@mp911de mp911de added the type: feature A new feature label Apr 8, 2024
@mp911de mp911de added this to the 7.x milestone Apr 8, 2024
@mp911de
Copy link
Collaborator

mp911de commented Apr 8, 2024

@atakavci @tishun feel free to merge.

@mp911de mp911de linked an issue Apr 8, 2024 that may be closed by this pull request
Copy link
Collaborator

@tishun tishun left a comment

Choose a reason for hiding this comment

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

LGTM

@tishun tishun merged commit c603c81 into redis:main Apr 8, 2024
5 of 6 checks passed
@tishun tishun assigned tishun and gerzse and unassigned tishun Apr 12, 2024
@tishun tishun modified the milestones: 7.x, 7.0.0.RELEASE, 6.4.0.RELEASE Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for HSCAN [NOVALUES]
4 participants