Conversation
Use byte-parser normalization for comparison and hashing while retaining permissive explicit conversions. Reject impossible numeric characters before renting scratch buffers for strings and fragmented sequences. Add storage, conversion, length and allocation regression coverage.
|
In the PR body (so: here) can we please be very clear and explicit exactly what scenario(s) this is addressing? Specifically, any change is a potential break to someone, so it would be good to be very clear to see what we're looking to change here. In particular, I wonder whether what we actually want here is an equality comparer instance, like |
|
If the point is to defer any lossy conversions until compare time: that's great, I just think we should be super clear about what actually changes in the PR body. |
| } | ||
| if (forComparison || seq.Length <= Format.MaxDoubleTextLen) | ||
| { | ||
| int len = checked((int)seq.Length); |
There was a problem hiding this comment.
I think we need to be mindful of what scenarios we need to allow/disallow here, and what means for the inputs; for example, if we can't simplify something that is over-long, it seems unlikely that we'd ever need the lease here - we can presumably discount such values immediately. So: what forComparison scenario forces this lease? Something with pathological numbers of leading zeros... probably isn't a useful case. I also wonder about [+-]inf etc.
There was a problem hiding this comment.
The lease preserves numeric comparison for overlong inputs, including leading zeroes; it isn't needed for inf, +inf, -inf or nan, which remain text for equality even though double casts can parse them. I tested removing it with a uniform 40-byte cutoff, but that also breaks existing long byte-array comparisons, so I haven't pushed that change. The body now gives the round-trip mismatch and breaking changes explicitly: would you prefer the bounded default behavior or an opt-in comparer?
This branch has not been deployed
2 participants
Fixes #3233. Today
(RedisValue)"1,000"compares equal to"1000", but not to its own UTF-8 bytes after a Redis round trip."(5)"and" 5 "have the same mismatch against"-5"and"5". A string-backedHashSet<RedisValue>can collapse these distinct server values, while a write/read round trip compares unequal.This PR changes default equality, hashing and numeric normalization during ordering to use the byte parser for all storage types.
"1,000"then equals its own bytes, not"1000"; its hash changes."5." == "5","+5" == "5"and"1e2" == "100"remain true. There is currently no numeric input length cutoff: long strings and segmented sequences that pass the numeric-character check rent a buffer before parsing. This also makes long numeric sequences normalize like contiguous bytes.These are breaking changes to default comparison, not just deferred conversions. Explicit numeric casts and
RedisValue.EqualityComparer.Binaryare unchanged. Special text such as"+inf"and"nan"does not equal numeric infinity or NaN; existing ordering fallbacks are unchanged and can still return zero for unequal values. This does not make default equality byte equality for all server values. Would an opt-in comparer be preferable to changing the default?The current head passed
dotnet build Build.csproj -c Release /p:CI=trueand 163 selected net10.0 value/parser tests. net8.0/full framework and Redis server integration were not run.Checklist