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 encode string as integer #39

Merged
merged 5 commits into from
Aug 26, 2024
Merged

fix encode string as integer #39

merged 5 commits into from
Aug 26, 2024

Conversation

michael2008
Copy link

@michael2008 michael2008 commented Aug 26, 2024

encoder should be strict about the strings that can be encoded as integer.
as strconv.ParseInt() is not accurate for this case, we need to check the string content and pick up
the "pure" uint32 or uint64 numbers.

Also the WriteSetObject() function has a wrong state to fix.

Fix issue #38
Fix issue #40

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

I will review it later today

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

Actually, -1 should be encoded as int

127.0.0.1:6379> set a -1
OK
127.0.0.1:6379> object encoding a
"int"

@michael2008
Copy link
Author

Actually, -1 should be encoded as int

127.0.0.1:6379> set a -1
OK
127.0.0.1:6379> object encoding a
"int"

I read some rdb libs where decoders read uint. so encoder better only encodes uint too.

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

For your information, redis use string2ll to determine whether int encoding can be used.

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

Actually, -1 should be encoded as int

127.0.0.1:6379> set a -1
OK
127.0.0.1:6379> object encoding a
"int"

I read some rdb libs where decoders read uint. so encoder better only encodes uint too.

Let me think about it

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

rdbtools supported signed int32 too. I prefer to follow the redis official strategy.

https://github.com/sripathikrishnan/redis-rdb-tools/blob/master/rdbtools/parser.py#L494

@michael2008
Copy link
Author

rdbtools supported signed int32 too. I prefer to follow the redis official strategy.

https://github.com/sripathikrishnan/redis-rdb-tools/blob/master/rdbtools/parser.py#L494

ok. but the encoder and decoder should be modified accordingly. current codes only support uint.

@HDT3213
Copy link
Owner

HDT3213 commented Aug 26, 2024

rdbtools supported signed int32 too. I prefer to follow the redis official strategy.
https://github.com/sripathikrishnan/redis-rdb-tools/blob/master/rdbtools/parser.py#L494

ok. but the encoder and decoder should be modified accordingly. current codes only support uint.

Although the code is a bit strange, it does work for negative numbers.😂 I'll sort it out.
https://github.com/HDT3213/rdb/blob/master/core/string.go#L92

@HDT3213 HDT3213 merged commit 76538c6 into HDT3213:master Aug 26, 2024
2 checks passed
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.

2 participants