Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9432 )
Change subject: [doc] Document the new decimal column type ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/9432/2/docs/developing.adoc File docs/developing.adoc: http://gerrit.cloudera.org:8080/#/c/9432/2/docs/developing.adoc@183 PS2, Line 183: , nit: remove this comma (even if you are a fan of the oxford comma, you only get to use it in lists of three or more) http://gerrit.cloudera.org:8080/#/c/9432/2/docs/known_issues.adoc File docs/known_issues.adoc: http://gerrit.cloudera.org:8080/#/c/9432/2/docs/known_issues.adoc@56 PS2, Line 56: * Type and nullability of existing columns cannot be changed by altering the table. maybe worth adding a note here that the precision and scale of DECIMAL columns can also not be changed http://gerrit.cloudera.org:8080/#/c/9432/2/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/9432/2/docs/release_notes.adoc@53 PS2, Line 53: * Kudu now supports a decimal column type. The decimal type is a numeric data type it's probably worth some note in compat section on what happens if you try to access a table with a DECIMAL column from an old client which doesn't understand it. Does it fail to access the table entirely, or only fail to access if you try to use that column, etc. http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc File docs/schema_design.adoc: http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@99 PS2, Line 99: float should we format 'float' and 'double' with `...` so it shows up more like a keyword? Or if you want to use the english name I would say "floating point types" rather than the specific keywords. Same with references to "The decimal type", perhaps should be "The `decimal` type" or somesuch? http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@100 PS2, Line 100: int64 same http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@103 PS2, Line 103: a precision and scale "a" seems misplaced http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@108 PS2, Line 108: For example, representing : integer values up to 9999, and fractional values up to 99.99, both require a : precision of 4 I think this would read a little more clearly written as: For example, a precision of 4 is required to represent integer values up to 9999, or to represent values up to 99.99 with two fractional digits. http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@116 PS2, Line 116: If precision and scale are equal, all the digits come after the decimal point, : making all the values between -0.999... and 0.999... This sentence is a bit confusing. Perhaps: If precision and scale are equal, all of the digits come after the decimal point. For example, a decimal with precision and scale equal to 3 can represent values between -0.999 and 0.999. http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@129 PS2, Line 129: * Decimal values with precision greater than 18 are stored in 16 bytes. Is it worth adding a note that we don't currently support BITSHUFFLE encoding for this last case and thus there is likely to be more than double the storage cost when moving from precision=18 to precision=19? It might also be worth adding some guidance reminding the user that precision and scale cannot be altered on existing data here, but warning them that "just use the highest precision possible" has some cost in performance, memory, and storage. http://gerrit.cloudera.org:8080/#/c/9432/2/docs/schema_design.adoc@143 PS2, Line 143: bitshuffle is bitshuffle the default for int128? I thought it wasnt supported but maybe you got that in while I wasn't paying attention :) -- To view, visit http://gerrit.cloudera.org:8080/9432 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9489613d35daad708648ea04d49e472d3149b33d Gerrit-Change-Number: 9432 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Alex Rodoni <arod...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 23 Feb 2018 21:01:17 +0000 Gerrit-HasComments: Yes
