sql/pgwire: fix encoding of decimals with trailing 0s #64022
Conversation
|
This change is |
| ---- | ||
| {"Type":"ParseComplete"} | ||
| {"Type":"BindComplete"} | ||
| {"Type":"DataRow","Values":[{"text":"10000"},{"text":"10000"}]} |
There was a problem hiding this comment.
can you add one for 10E4 too?
There was a problem hiding this comment.
you mean add an expected return value of 10E4? i don't think i understand -- postgres will never return a decimal value with that text format (and this is the noncrdb_only expected result)
is 10E4 what we would get if we add 0000 to the encoding above? sorry i don't understand!
There was a problem hiding this comment.
done! i think that shows why CRDB uses scientific notation -- it lets us preserve the number of significant digits (as detailed in #17029)
|
|
||
| send | ||
| Parse {"Name": "s1", "Query": "SELECT 10000::decimal, $1::decimal"} | ||
| Bind {"DestinationPortal": "p1", "PreparedStatement": "s1", "ParameterFormatCodes": [1], "Parameters": [[0, 1, 0, 1, 0, 0, 0, 0, 0, 1]]} |
There was a problem hiding this comment.
can you also add a test that sends an extra 0000 at the end? (see my second bullet point for #56907 (comment))
There was a problem hiding this comment.
i may not understand -- where should i add the 0000?
When CockroachDB decodes the value NPGSQL provides, I think we will return 10000 instead of 1E+4 if you make add extra 00 00 at the end (other drivers do this so perhaps this is the problem). I think 10000 0000 is broken too, etc. for any value with 0000 as the last 4 digits.
Meaning add 00 00 to the end of the binary parameter passed in?
There was a problem hiding this comment.
Yeah sorry I mean if we do introduce the extra 0000 to read but maybe I misread my old comment
There was a problem hiding this comment.
hm yeah if i add 0000 to the end of the binary param, it is rejected as invalid binary data
rafiss
force-pushed
the
fix-decimal-0000-encoding
branch
from
08e040c to
3f92175
Compare
rafiss
force-pushed
the
fix-decimal-0000-encoding
branch
from
3f92175 to
1a35cc6
Compare
The dscale is defined as number of digits (in base 10) visible after the decimal separator," so a negative dscale makes no sense and is not valid. This was preventing NPGSQL from being able to decode the binary decimals that CockroachDB was sending. Release note (bug fix): The binary encoding of decimals no longer will have negative `dscale` values. This was preventing Npgsql from being able to read some binary decimals from CockroachDB.
rafiss
force-pushed
the
fix-decimal-0000-encoding
branch
from
1a35cc6 to
f7fe860
Compare
|
tftr! bors r=otan |
|
Build failed (retrying...): |
|
Build succeeded: |
fixes #56907
The dscale is defined as number of digits (in base 10)
visible after the decimal separator," so a negative
dscale makes no sense and is not valid.
This was preventing NPGSQL from being able to decode the binary decimals
that CockroachDB was sending.
Release note (bug fix): The binary encoding of decimals no longer will
have negative
dscalevalues. This was preventing Npgsql from beingable to read some binary decimals from CockroachDB.