Fixed decimal encoding in a backwards compatible way #302
Conversation
|
@rockwotj another review when you get a chance. The CI failed but it seems to be an unrelated issue |
logical_type.go
Outdated
| } | ||
|
|
||
| // Check if bytes look like ASCII decimal string (backwards compat) | ||
| if looksLikeASCIIDecimal(bs) { |
There was a problem hiding this comment.
I do not think this is backwards compatible, are we sure there is never valid encoding of a number that would look like valid ascii? I don’t believe so. We need an option for this unless you have some proof this is never wrong
There was a problem hiding this comment.
You're right... I have added an option and reduced the scope of the heuristic for determining that the bytes are a valid ascii..
... Or alternatively, we could have a compatibility mode (by default) which can be overridden to false so the encoding is modified.
There was a problem hiding this comment.
@rockwotj ok decided to go with fully backwards compatible option and NOT try to recover badly encoded data (since it's imperfect logic). So enabling the spec compliant option will switch to the new mode...
… use cases (i.e. has a `.`)
2 participants
Description
Opt-in spec compliant encoding
1. Add new codec option
EnableDecimalBinarySpecCompliantEncodingtoCodecOptionin codec.gofalse(legacy behavior)2. Conditionally use spec-compliant encoding
true: Use proper two's complement binary encoding and human-readable textualfalse: Use legacy ASCII string encoding (original behavior)Issue Reference
#296