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 <[email protected]>
Signed-off-by: Xuyang Wang <[email protected]>
Signed-off-by: Xuyang Wang <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Dec 1, 2024

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Xuyang Wang <[email protected]>
Signed-off-by: Xuyang Wang <[email protected]>
@Nugine Nugine mentioned this pull request Dec 1, 2024
3 tasks
Signed-off-by: Xuyang Wang <[email protected]>
@Nugine Nugine marked this pull request as ready for review December 3, 2024 01:51
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
@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
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
The bug was introduced in redis#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 <[email protected]>
Co-authored-by: debing.sun <[email protected]>
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