Skip to content

Conversation

Nugine
Copy link
Contributor

@Nugine Nugine commented Dec 1, 2024

The bug was introduced in #13558 .

When merging dense hll structures, hllDenseCompress writes to wrong location and the result will be zero. The unit tests didn't cover this case.

This PR

  • fixes the bug
  • adds PFDEBUG SIMD (ON|OFF) for unit tests
  • adds a new TCL test to cover the cases

Synchronized from valkey-io/valkey#1293

Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
@CLAassistant
Copy link

CLAassistant commented Dec 1, 2024

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
@Nugine Nugine mentioned this pull request Dec 1, 2024
3 tasks
Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
@Nugine Nugine marked this pull request as ready for review December 3, 2024 01:51
Co-authored-by: debing.sun <debing.sun@redis.com>
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

LGTM, @sundb do we need to add release-notes tag for this, as it introduces PFDEBUG SIMD (ON|OFF) subcommand

@sundb
Copy link
Collaborator

sundb commented Dec 10, 2024

@ShooterIT no, because #13558 doesn't release.

@ShooterIT
Copy link
Member

I don't mean we mention bug fix instead of new sub command

@sundb
Copy link
Collaborator

sundb commented Dec 10, 2024

@ShooterIT you're right, we should add release note label for sub command.

@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 11, 2024
@ShooterIT
Copy link
Member

Hi @YaacovHazan I want to merge this PR. Even this PR introduces a subcommand pfdebug simd, but it seems we don't want to developer to use it, https://redis.io/docs/latest/commands/pfdebug/, right?

@ShooterIT ShooterIT merged commit 6840776 into redis:unstable Dec 18, 2024
17 checks passed
YaacovHazan pushed a commit that referenced this pull request Jan 14, 2025
The bug was introduced in #13558 . 

When merging dense hll structures, `hllDenseCompress` writes to wrong
location and the result will be zero. The unit tests didn't cover this
case.

This PR
+ fixes the bug
+ adds `PFDEBUG SIMD (ON|OFF)` for unit tests
+ adds a new TCL test to cover the cases

Synchronized from valkey-io/valkey#1293

---------

Signed-off-by: Xuyang Wang <xuyangwang@link.cuhk.edu.cn>
Co-authored-by: debing.sun <debing.sun@redis.com>
@sundb sundb added this to Redis 8.0 Aug 15, 2025
@sundb sundb removed this from Redis 8.2 Aug 15, 2025
@sundb sundb moved this from Todo to Done in Redis 8.0 Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants