Update INT docs re: INT8, default_int_size setting #4317
Conversation
|
This change is |
rmloveland
force-pushed
the
20190130-int8-updates
branch
from
38f1e93
to
1df9b86
Compare
|
Direct link to updated page for easier reference: |
|
Thanks for the review @bobvawter ! |
There was a problem hiding this comment.
LGTM, with some minor grammar stuff
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @lhirata and @rmloveland)
v2.2/int.md, line 31 at r1 (raw file):
The different integer types place different constraints on the range of allowable values, but all integers are stored in the same way regardless of type. Smaller values take up less space than larger ones (based on the numeric value, not the data type). By default, `INT` is an alias for `INT8`, which creates 64-bit signed integers. This differs from the Postgres default for `INT`, [which is 32 bits](https://www.postgresql.org/docs/9.6/datatype-numeric.html).
Remove double space (and elsewhere)
v2.2/int.md, line 35 at r1 (raw file):
This may cause issues for your application if it is not written to handle 64-bit integers, whether due to the language your application is written in, or the ORM/framework it uses to generate SQL (if any). For example, JavaScript language runtimes represent numbers as 64-bit floats, which means that the JS runtime can only represent 53 bits of numeric accuracy and thus has a max safe value of 2<sup>53</sup>, or 9007199254740992. For more information, see the article [How numbers are encoded in JavaScript](http://2ality.com/2012/04/number-encoding.html) by Dr. Axel Rauschmayer.
Add quotes around article name
v2.2/int.md, line 44 at r1 (raw file):
This means that if a table contains a column with a default-sized
INTvalue, and you are reading from it / writing to it via JavaScript, you will not be able to read and write values to that column correctly. This issue can pop up in a surprising way if you are using a framework that autogenerates both frontend and backend code (such as twirp). In such cases, you may find that your backend code can handle 64-bit signed integers, but the generated client/frontend code cannot.
I would reword this to exclude the slash- "[...] and you are reading from or writing to it via JavaScript"
There was a problem hiding this comment.
Thanks for the review Lauren! All comments should be addressed.
Reviewable status:
v2.2/int.md, line 31 at r1 (raw file):
Previously, lhirata wrote…
Remove double space (and elsewhere)
Done. All instances changed to single space.
v2.2/int.md, line 35 at r1 (raw file):
Previously, lhirata wrote…
Add quotes around article name
Done.
v2.2/int.md, line 44 at r1 (raw file):
Previously, lhirata wrote…
I would reword this to exclude the slash- "[...] and you are reading from or writing to it via JavaScript"
Done.
rmloveland
force-pushed
the
20190130-int8-updates
branch
from
1df9b86
to
38e46b4
Compare
There was a problem hiding this comment.
Reviewable status:
v2.2/int.md, line 35 at r1 (raw file):
The "For more info" sentence is interrupting the flow here. Can you make the JS runtime can only represent 53 bits of numeric accuracy a link to that number-encoding.html article? If so, the next paragraph could be merged with this one to flow better:
For example, JavaScript language runtimes represent numbers as 64-bit floats, which equals only 53 bits of numeric accuracy](http://2ality.com/2012/04/number-encoding.html), and thus has a max safe value of 253, or 9007199254740992. This means that the maximum size of a default
INTin CockroachDB is much larger than JavaScript can represent as an integer. Visually, the size difference is as follows:
v2.2/int.md, line 31 at r2 (raw file):
The different integer types place different constraints on the range of allowable values, but all integers are stored in the same way regardless of type. Smaller values take up less space than larger ones (based on the numeric value, not the data type). By default, `INT` is an alias for `INT8`, which creates 64-bit signed integers. This differs from the Postgres default for `INT`, [which is 32 bits](https://www.postgresql.org/docs/9.6/datatype-numeric.html).
This is great, but I think we might be able to break up this content a little with an h3 sub-heading. What about Considerations for 64-bit signed integers?
v2.2/int.md, line 33 at r2 (raw file):
nit: I think we can combine a few paragraphs to cut down on use of "This". For example, an option here would be to merge this with the previous paragraph:
By default,
INTis an alias forINT8, which creates 64-bit signed integers. This differs from the Postgres default forINT, which is 32 bits, and may cause issues for your application if it is not written to handle 64-bit integers...
v2.2/int.md, line 44 at r2 (raw file):
This means that if a table contains a column with a default-sized
INTvalue, and you are reading from it or writing to it via JavaScript, you will not be able to read and write values to that column correctly. This issue can pop up in a surprising way if you are using a framework that autogenerates both frontend and backend code (such as twirp). In such cases, you may find that your backend code can handle 64-bit signed integers, but the generated client/frontend code cannot.
I'm trying to find ways to not say "This means" too often. Here, an option would be Given the above, if a table contains....
Fixes #4183, #4254. Lays some groundwork for future work on #4298. Summary of changes: - Update `INT` page to include examples of actual min/max integers supported by each type for easier reference. - Update `INT` data type page with additions to the **Size** section, specifically: - A description of possible compatibility issues caused by 64-bit integers vs. e.g. JavaScript runtimes, including when using frameworks that autogenerate frontend code. - Mention and link to `default_int_size` session var and cluster setting. - Add `default_int_size` to session vars page
There was a problem hiding this comment.
Thanks for the review Jesse! Addressed all your comments as recommended.
Reviewable status:
v2.2/int.md, line 35 at r1 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
The "For more info" sentence is interrupting the flow here. Can you make
the JS runtime can only represent 53 bits of numeric accuracya link to that number-encoding.html article? If so, the next paragraph could be merged with this one to flow better:For example, JavaScript language runtimes represent numbers as 64-bit floats, which equals only 53 bits of numeric accuracy](http://2ality.com/2012/04/number-encoding.html), and thus has a max safe value of 253, or 9007199254740992. This means that the maximum size of a default
INTin CockroachDB is much larger than JavaScript can represent as an integer. Visually, the size difference is as follows:
Done.
v2.2/int.md, line 31 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This is great, but I think we might be able to break up this content a little with an h3 sub-heading. What about
Considerations for 64-bit signed integers?
Done. Added a subheading with that title.
v2.2/int.md, line 33 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
nit: I think we can combine a few paragraphs to cut down on use of "This". For example, an option here would be to merge this with the previous paragraph:
By default,
INTis an alias forINT8, which creates 64-bit signed integers. This differs from the Postgres default forINT, which is 32 bits, and may cause issues for your application if it is not written to handle 64-bit integers...
Done.
v2.2/int.md, line 44 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'm trying to find ways to not say "This means" too often. Here, an option would be
Given the above, if a table contains....
Done.
rmloveland
force-pushed
the
20190130-int8-updates
branch
from
38e46b4
to
108f335
Compare
Fixes #4183, #4254.
Lays some groundwork for future work on #4298.
Summary of changes:
Update
INTpage to include examples of actual min/max integerssupported by each type for easier reference.
Update
INTdata type page with additions to the Sizesection, specifically:
A description of possible compatibility issues caused by 64-bit
integers vs. e.g. JavaScript runtimes, including when using
frameworks that autogenerate frontend code.
Mention and link to
default_int_sizesession var and clustersetting.
Add
default_int_sizeto session vars page