You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2016/11/15 00:57:20 UTC
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Dan Hecht has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5083
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
IMPALA-4436: StringValue::StringCompare() should match strncmp()
According to the C standard, strncmp() interprets characters as
unsigned, whereas StringCompare() uses char (which happens to be
signed). This means that for values greater than 127, they don't give
the same result (which is especially bad considering StringCompare()
falls back to strncmp(), and so the answer will depends on the
mismatched position).
Fix StringCompare() to interpret as unsigned char.
Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/string-compare-test.cc
M be/src/runtime/string-value.inline.h
3 files changed, 70 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/5083/1
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5083/1//COMMIT_MSG
Commit Message:
Line 13: falls back to strncmp(), and so the answer depends on the mismatched
> typo: will depends
Done
http://gerrit.cloudera.org:8080/#/c/5083/1/be/src/runtime/string-compare-test.cc
File be/src/runtime/string-compare-test.cc:
Line 35: TEST(StringCompareTest, Basic) {
> There's a TestCompare in string-value-test.cc. Have you considered putting
This new test is verify the layer below -- i.e. StringCompare() return value vs. strncmp, whereas that code is verifying Eq() etc methods of StringValue.
http://gerrit.cloudera.org:8080/#/c/5083/1/be/src/runtime/string-value.inline.h
File be/src/runtime/string-value.inline.h:
Line 51: return static_cast<unsigned char>(s1[chars_match]) -
> static_cast?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5083/1//COMMIT_MSG
Commit Message:
Line 13: falls back to strncmp(), and so the answer will depends on the
typo: will depends
http://gerrit.cloudera.org:8080/#/c/5083/1/be/src/runtime/string-compare-test.cc
File be/src/runtime/string-compare-test.cc:
Line 35: TEST(StringCompareTest, Basic) {
There's a TestCompare in string-value-test.cc. Have you considered putting the test there?
http://gerrit.cloudera.org:8080/#/c/5083/1/be/src/runtime/string-value.inline.h
File be/src/runtime/string-value.inline.h:
Line 51: return (unsigned char)s1[chars_match] - (unsigned char)s2[chars_match];
static_cast?
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has uploaded a new patch set (#2).
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
IMPALA-4436: StringValue::StringCompare() should match strncmp()
According to the C standard, strncmp() interprets characters as
unsigned, whereas StringCompare() uses char (which happens to be
signed). This means that for values greater than 127, they don't give
the same result (which is especially bad considering StringCompare()
falls back to strncmp(), and so the answer depends on the mismatched
position).
Fix StringCompare() to interpret as unsigned char.
Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/string-compare-test.cc
M be/src/runtime/string-value.inline.h
3 files changed, 71 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/5083/2
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
Patch Set 3: Code-Review+2
Carry and promote Alex's +1.
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4436: StringValue::StringCompare() should match strncmp()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4436: StringValue::StringCompare() should match strncmp()
......................................................................
IMPALA-4436: StringValue::StringCompare() should match strncmp()
According to the C standard, strncmp() interprets characters as
unsigned, whereas StringCompare() uses char (which happens to be
signed). This means that for values greater than 127, they don't give
the same result (which is especially bad considering StringCompare()
falls back to strncmp(), and so the answer depends on the mismatched
position).
Fix StringCompare() to interpret as unsigned char.
Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Reviewed-on: http://gerrit.cloudera.org:8080/5083
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/string-compare-test.cc
M be/src/runtime/string-value.inline.h
3 files changed, 71 insertions(+), 1 deletion(-)
Approvals:
Internal Jenkins: Verified
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5083
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic0750f98d8c5ef7d0c0ea279cd1f80b4acbad1be
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins