Support special IEEE754 values serdes in tables + fix some serdes issues with non-string dictionaries #92
Open
Support special IEEE754 values serdes in tables + fix some serdes issues with non-string dictionaries #92
Conversation
toptobes
force-pushed
the
KG-special-ieee754-values
branch
from
ca859e3 to
3516365
Compare
sl-at-ibm
reviewed
Mar 16, 2026
| _ => throw new JsonException($"Unexpected string value '{reader.GetString()}' for double type") // should never actually happen | ||
| }; | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
nit: 8 whitespaces to remove
sl-at-ibm
reviewed
Mar 16, 2026
| { | ||
| return reader.GetDouble(); | ||
| } | ||
|
|
sl-at-ibm
reviewed
Mar 16, 2026
| _ => throw new JsonException($"Unexpected string value '{reader.GetString()}' for float type") // should never actually happen | ||
| }; | ||
| } | ||
|
|
sl-at-ibm
reviewed
Mar 16, 2026
| { | ||
| return reader.GetSingle(); | ||
| } | ||
|
|
toptobes
force-pushed
the
KG-special-ieee754-values
branch
from
3516365 to
a3f1b45
Compare
toptobes
force-pushed
the
KG-special-ieee754-values
branch
from
a3f1b45 to
33fb557
Compare
toptobes
changed the title
Nan, Infinity and -Infinity when reading/writing floats/doubles
toptobes
force-pushed
the
KG-special-ieee754-values
branch
2 times, most recently
from
d5bbc92 to
e1a6a6a
Compare
toptobes
force-pushed
the
KG-special-ieee754-values
branch
from
e1a6a6a to
a05be67
Compare
toptobes
force-pushed
the
KG-special-ieee754-values
branch
from
bdec3b0 to
ed7c3bb
Compare
sl-at-ibm
reviewed
Mar 18, 2026
| { | ||
| return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(Dictionary<,>); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Nit: spurious whitespaces on lines: 31 41 46 49 54 79.
sl-at-ibm
reviewed
Mar 18, 2026
| } | ||
|
|
||
| [Fact] | ||
| public async Task SteoNonstringMapTableInsertionTests() |
Collaborator
There was a problem hiding this comment.
While I'd like my name to remain in the csharp client history, I'm also humble enough to suggest the name of this test (and the associated table) be edited to a more serious choice :p
sl-at-ibm
approved these changes
Mar 18, 2026
Collaborator
sl-at-ibm
left a comment
sl-at-ibm
left a comment
There was a problem hiding this comment.
LGTM
Minor remarks if you can spare a few minutes
- whitespaces
- rename "steo" tests (lol)
- the "XML comments" warnings seem all fine to me. There must be a way to silence them (but without missing the important stuff)...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IMPORTANT: This PR swallowed #95
Resolves #65 and fixes #96
I would like to use an
IFloatingPointIeee754-based solution to deduplicate[Float|Double]Converterbut I think I need to use C#11+ to do that...