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

fix(hll_family): Fixed PFMERGE wrong merge operation #4796

Merged
merged 2 commits into from
Mar 20, 2025
Merged

Conversation

mkaruza
Copy link
Contributor

@mkaruza mkaruza commented Mar 18, 2025

Fixed bug in hllMergeDense that writes to wrong register.

Fixes #4750

@mkaruza mkaruza force-pushed the mkaruza/github#4750 branch from c4c3936 to 51e83ca Compare March 18, 2025 10:42
@romange romange requested a review from adiholden March 18, 2025 11:13
@mkaruza mkaruza force-pushed the mkaruza/github#4750 branch from 51e83ca to 6481c76 Compare March 18, 2025 12:02
@Niennienzz
Copy link
Contributor

I don't think that I understand the code fully, haha, I am just reading the PR title... Just want to confirm that the destination key should be treated as a source if it exists. And because HLL should always count unique values, that's why the examples I made in #4750 should not accumulate counts from destination. But it doesn't change the fact that destination is still a source. Here are more examples:

$> PFADD destination 1 2 3
(integer) 1
$> PFMERGE destination     # 'destination' is a source, even with no additional keys to merge.
OK
$> PFCOUNT destination
(integer) 3

$> PFADD key 4 5
(integer) 1
$> PFMERGE destination key # This is merging 'destination' and 'key', and saving the result to 'destination'.
OK
$> PFCOUNT destination
(integer) 5

Please ignore if the behaviour is already correct. I just wanted to confirm.

@mkaruza mkaruza force-pushed the mkaruza/github#4750 branch from 6481c76 to a84f964 Compare March 20, 2025 10:01
Fixed bug in hllMergeDense that writes to wrong register.

Fixes #4750

Signed-off-by: mkaruza <[email protected]>
@mkaruza mkaruza force-pushed the mkaruza/github#4750 branch from a84f964 to 787b487 Compare March 20, 2025 10:02
@mkaruza mkaruza changed the title fix(hll_family): Don't use destination key in PFMERGE operation fix(hll_family): Fixed PFMERGE wrong merge operation Mar 20, 2025
@mkaruza
Copy link
Contributor Author

mkaruza commented Mar 20, 2025

Thank you @Niennienzz i have updated PR so it treats destination as valid source if exits (and fixes merge operation to produce correct values)

@mkaruza mkaruza merged commit 555d6b5 into main Mar 20, 2025
10 checks passed
@mkaruza mkaruza deleted the mkaruza/github#4750 branch March 20, 2025 20:34
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.

PFMERGE wrong behavior
3 participants