You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Laurel Hale (Code Review)" <ge...@cloudera.org> on 2017/06/07 00:03:23 UTC
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Laurel Hale has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7098
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
IMPALA-3603 [DOCS] Document handling of NaN values
Added information in the "DOUBLE Data Type" (impala_double.html)
and the "FLOAT Data Type" (impala_float.html) topics about
how Impala handles NaN values.
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':
SELECT CAST('nan' AS DOUBLE|FLOAT)=CAST('nan' AS DOUBLE|FLOAT);
Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
---
M docs/topics/impala_double.xml
M docs/topics/impala_float.xml
2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/7098/1
--
To view, visit http://gerrit.cloudera.org:8080/7098
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Laurel Hale (Code Review)" <ge...@cloudera.org>.
Laurel Hale has uploaded a new patch set (#3).
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
IMPALA-3603 [DOCS] Document handling of NaN values
Added information in the "DOUBLE Data Type" (impala_double.html)
and the "FLOAT Data Type" (impala_float.html) topics about
how Impala handles NaN values.
Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
---
M docs/shared/impala_common.xml
M docs/topics/impala_double.xml
M docs/topics/impala_float.xml
3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/7098/3
--
To view, visit http://gerrit.cloudera.org:8080/7098
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <la...@cloudera.com>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-docs-submit/143/
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
Patch Set 3: Verified+1
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Laurel Hale (Code Review)" <ge...@cloudera.org>.
Laurel Hale has posted comments on this change.
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
Patch Set 2:
(9 comments)
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 an
okay.
PS2, Line 23: For patch set #2
> The commit message shouldn't reference the revisions of the patch. It's mea
Thanks.
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.
Done
PS2, Line 2300: (not a number)
> Redundant.
I disagree about including "(not a number)." It's a documentation best practice to always provide what an acronym stands for on first use in a topic to avoid ambiguity.
I agree with removing "values" and will do that.
PS2, Line 2302: returns
> Consider "evaluates to" rather than "returns"...I slightly prefer the forme
I think the subordinate clause is important for clarity and so will leave as is.
PS2, Line 2301: , which evaluates equality
: between two NaN values
> Redundant.
I disagree. Will leave it in.
http://gerrit.cloudera.org:8080/#/c/7098/2/docs/topics/impala_double.xml
File docs/topics/impala_double.xml:
PS2, Line 80:
> Extra whitespace.
Done
http://gerrit.cloudera.org:8080/#/c/7098/2/docs/topics/impala_float.xml
File docs/topics/impala_float.xml:
PS2, Line 74:
> Extra whitespace.
Done
PS2, Line 76:
> Extra whitespace.
Done
--
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 <la...@cloudera.com>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
Patch Set 3: Code-Review+2
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
IMPALA-3603 [DOCS] Document handling of NaN values
Added information in the "DOUBLE Data Type" (impala_double.html)
and the "FLOAT Data Type" (impala_float.html) topics about
how Impala handles NaN values.
Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
Reviewed-on: http://gerrit.cloudera.org:8080/7098
Reviewed-by: Michael Brown <mi...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M docs/shared/impala_common.xml
M docs/topics/impala_double.xml
M docs/topics/impala_float.xml
3 files changed, 22 insertions(+), 0 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Brown: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7098
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[Impala-ASF-CR] IMPALA-3603 [DOCS] Document handling of NaN values
Posted by "Laurel Hale (Code Review)" <ge...@cloudera.org>.
Laurel Hale has uploaded a new patch set (#2).
Change subject: IMPALA-3603 [DOCS] Document handling of NaN values
......................................................................
IMPALA-3603 [DOCS] Document handling of NaN values
Added information in the "DOUBLE Data Type" (impala_double.html)
and the "FLOAT Data Type" (impala_float.html) topics about
how Impala handles NaN values.
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':
SELECT CAST('nan' AS DOUBLE|FLOAT)=CAST('nan' AS DOUBLE|FLOAT);
For patch set #2, I tidied up a bit. I pulled the common
verbiage into the /shared/impala_common.xml file and
added the Jira number in a comment for auditing purposes.
Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
---
M docs/shared/impala_common.xml
M docs/topics/impala_double.xml
M docs/topics/impala_float.xml
3 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/7098/2
--
To view, visit http://gerrit.cloudera.org:8080/7098
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id9485b6790d58fafdae32332d2634cbe893d7fb0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale <la...@cloudera.com>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>