You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/02/23 19:49:55 UTC

[kudu-CR] [doc] Document the new decimal column type

Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9432


Change subject: [doc] Document the new decimal column type
......................................................................

[doc] Document the new decimal column type

Change-Id: I9489613d35daad708648ea04d49e472d3149b33d
---
M docs/developing.adoc
M docs/known_issues.adoc
M docs/kudu_impala_integration.adoc
M docs/release_notes.adoc
M docs/schema_design.adoc
5 files changed, 50 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/9432/1
-- 
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: newchange
Gerrit-Change-Id: I9489613d35daad708648ea04d49e472d3149b33d
Gerrit-Change-Number: 9432
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@gmail.com>

[kudu-CR] [doc] Document the new decimal column type

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

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 4: Code-Review+2


-- 
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: 4
Gerrit-Owner: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 12 Mar 2018 23:47:30 +0000
Gerrit-HasComments: No

[kudu-CR] [doc] Document the new decimal column type

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

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 <gr...@gmail.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Feb 2018 21:01:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] [doc] Document the new decimal column type

Posted by "Alex Rodoni (Code Review)" <ge...@cloudera.org>.

Alex Rodoni has posted comments on this change. ( http://gerrit.cloudera.org:8080/9432 )

Change subject: [doc] Document the new decimal column type
......................................................................


Patch Set 2:

(1 comment)

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: a
the



-- 
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 <gr...@gmail.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Feb 2018 22:14:17 +0000
Gerrit-HasComments: Yes