Will Berkeley has posted comments on this change. Change subject: IMPALA-3603 [DOCS] Document handling of NaN values ......................................................................
Patch Set 2: (10 comments) There's a few instances of extra whitespace. There should be an editor configuration in whatever editor you use that will keep it from sneaking in (happens to everyone though). http://gerrit.cloudera.org:8080/#/c/7098/2//COMMIT_MSG Commit Message: PS2, Line 13: This information has been added to the subsection : "Usage Notes:" in each topic: : : "Impala does not evaluate NaN (not a number) values as : equal to any other numeric values, including NaN. For : example, the following statement, which evaluates : equality between two NaN values returns 'false': The commit message doesn't need to be this detailed. I would just add a another sentence or so after your first to say what the change was and why it was (should be) changed: "Added information in the "DOUBLE Data Type" (impala_double.html) and the "FLOAT Data Type" (impala_float.html) topics to clarify that Impala does not evaluate NaN as equal to any other floating point number. This behavior, while consistent with IEEE 754, might be confusing or surprising to users because some well-known databases like postgres behave differently." PS2, Line 23: For patch set #2 The commit message shouldn't reference the revisions of the patch. It's meant to be the summary of the change that someone will read sometime in the future so they can quickly understand what was changed and why. The n revisions the patch went through aren't relevant. If you want to detail what you changed between patch sets, comment on the review. http://gerrit.cloudera.org:8080/#/c/7098/2/docs/shared/impala_common.xml File docs/shared/impala_common.xml: PS2, Line 2298: Extra whitespace. Remove. PS2, Line 2300: values I would remove and just say "Impala does not evaluate NaN as equal to any other numeric value, including other NaN value" PS2, Line 2300: (not a number) Redundant. PS2, Line 2301: , which evaluates equality : between two NaN values Redundant. PS2, Line 2302: returns Consider "evaluates to" rather than "returns"...I slightly prefer the former to the latter, but would be OK with either. http://gerrit.cloudera.org:8080/#/c/7098/2/docs/topics/impala_double.xml File docs/topics/impala_double.xml: PS2, Line 80: Extra whitespace. http://gerrit.cloudera.org:8080/#/c/7098/2/docs/topics/impala_float.xml File docs/topics/impala_float.xml: PS2, Line 74: Extra whitespace. PS2, Line 76: Extra whitespace. -- To view, visit http://gerrit.cloudera.org:8080/7098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Laurel Hale <lau...@cloudera.com> Gerrit-Reviewer: John Russell <jruss...@cloudera.com> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
