Skip to content

Conversation

clkbug
Copy link
Contributor

@clkbug clkbug commented Apr 14, 2023

I have fixed issue 211.
This changed key_stddev and key_median to read as unsigned long long.
In real-world use cases, the key ranges will be wide enough that the fractional portions will not matter.

case o_key_stddev:
endptr = NULL;
cfg->key_stddev = (unsigned int) strtof(optarg, &endptr);
cfg->key_stddev = strtoull(optarg, &endptr, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the loss of fractional part. Specifically in stddev.
But anyway I think the problem is just the wrong use of 32bit float instead of double (although even 32bit float can hold values much larger than 32bit integer).
I.e. strtod should solve it and keep both thr fractional part and a huge range.

Copy link
Contributor

Choose a reason for hiding this comment

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

i missed the fact that we had casting ((unsigned int)), so we didn't have fractional part anyway.
but that casting is actually the problem (not the strtof), so it was maybe enough to just drop the casting.
but if we change these lines, let's got with strtod (changing to strtoull would be a breaking change, since old commands that used to work, will now error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand.
I have changed it to use strtod.

@clkbug clkbug force-pushed the allow-larger-key-median-and-stddev branch from 2bf9737 to c5f2ddc Compare April 17, 2023 13:03
@YaacovHazan YaacovHazan merged commit d6083ed into RedisLabs:master Apr 20, 2023
@YaacovHazan
Copy link
Collaborator

@clkbug Thank you

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.

3 participants