impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Will Berkeley (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Date Wed, 07 Jun 2017 23:25:02 GMT
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 <laurel@cloudera.com>
Gerrit-Reviewer: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wdberkeley@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message