[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


IMPALA-3308: Get expr-test passing on PPC64LE

When using gcc 5+ (which introduced a new library ABI that includes new
implementations of std::string) to build Impala, the copy of the class
ExprValue(std::string) would be unsafe as string_val.ptr will not be
updated to point to the relocated string_data.data().

In order to solve this issue, we need to change how we initialize value_
so that it is initialized in-place, rather than created as a temporary
on the stack and then copied.

Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Reviewed-on: http://gerrit.cloudera.org:8080/4186
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
2 files changed, 11 insertions(+), 12 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 6: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 6: Code-Review+2

Rebase, carry +2

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 5:

I'll merge this

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 4:

Let's also disallow the copy constructor (see DISALLOW_COPY_AND_ASSIGN macro).

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

PS3, Line 73: string_val.len = string_data.size();
> Ack, sorry for the red herring.
No problem, it's good to have eyes on these things. The original bug lurked 
here for a long time.


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[segelyang (Code Review)]

segelyang has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 4:

Thanks for your comments. I have re-uploaded the patch.

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[segelyang (Code Review)]

segelyang has uploaded a new patch set (#4).

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..

IMPALA-3308: Get expr-test passing on PPC64LE

When using gcc 5+ (which introduced a new library ABI that includes new
implementations of std::string) to build Impala, the copy of the class
ExprValue(std::string) would be unsafe as string_val.ptr will not be
updated to point to the relocated string_data.data().

In order to solve this issue, we need to change how we initialize value_
so that it is initialized in-place, rather than created as a temporary
on the stack and then copied.

Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
---
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
2 files changed, 9 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/4186/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

PS3, Line 73: string_val.ptr = const_cast(string_data.data());
> string_data looks like it's private (string_val is public).
Ack, sorry for the red herring.


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

PS3, Line 73: string_val.ptr = const_cast(string_data.data());
> In that case, can string_data be protected, rather than public? We could st
string_data looks like it's private (string_val is public).


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

PS3, Line 73: string_val.ptr = const_cast(string_data.data());
> There's a non-const char version http://www.cplusplus.com/reference/string/
In that case, can string_data be protected, rather than public? We could still 
expose a const reference to it as public.


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

PS3, Line 73: string_val.ptr = const_cast(string_data.data());
> Does this have the possibility of creating Undefined Behavior?
The c++11 standard guarantees that string storage is contiguous. Should be fine 
as long as the string isn't modified later.

Let's change this to string_val.ptr = _data[0] to avoid the const_cast.


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

(2 comments)

No problems, just wanted to make sure that this patch made it it.

http://gerrit.cloudera.org:8080/#/c/4186/3/be/src/exprs/expr-value.h
File be/src/exprs/expr-value.h:

Line 70:   /// Init for string values
Can you remove this comment? It's not helpful (I know you didn't add it :)).


Line 71:   void Init(const std::string& str){
Can you fix the missing whitespace in this line and the one below. Probably the 
easiest is to run clang-format on the patch (we recently added a .clang-format 
file). This command will run clang-format on the lines changed in the most 
recent commit on your branch:

  git diff -U0 HEAD^  | 
$IMPALA_TOOLCHAIN/llvm-3.8.0-p1/share/clang/clang-format-diff.py -i -p1


-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[segelyang (Code Review)]

segelyang has posted comments on this change.

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..


Patch Set 3:

I had a holiday last week and am sorry for late. I have uploaded a new patch. 
Please help to review.

-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3308: Get expr-test passing on PPC64LE

[segelyang (Code Review)]

segelyang has uploaded a new patch set (#3).

Change subject: IMPALA-3308: Get expr-test passing on PPC64LE
..

IMPALA-3308: Get expr-test passing on PPC64LE

When using gcc 5+ (which introduced a new library ABI that includes new
implementations of std::string) to build Impala, the copy of the class
ExprValue(std::string) would be unsafe as string_val.ptr will not be
updated to point to the relocated string_data.data().

In order to solve this issue, we need to change how we initialize value_
so that it is initialized in-place, rather than created as a temporary
on the stack and then copied.

Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
---
M be/src/exprs/expr-value.h
M be/src/exprs/literal.cc
2 files changed, 9 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/4186/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4186
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4504ee6a52a085f530aadfcfa009bacb83c64787
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: segelyang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: segelyang