You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2016/07/27 23:40:16 UTC

[Impala-ASF-CR] CDH-41931: Toolchain for Ubuntu16 Impala builds 

Thomas Tauber-Marshall has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/3800

Change subject: CDH-41931: Toolchain for Ubuntu16 Impala builds
......................................................................

CDH-41931: Toolchain for Ubuntu16 Impala builds

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <math.h>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <math.h> imports to all files that use those functions, for
the sake of explicitness.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
5 files changed, 18 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/3800/1
-- 
To view, visit http://gerrit.cloudera.org:8080/3800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] CDH-41931: Toolchain for Ubuntu16 Impala builds

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: CDH-41931: Toolchain for Ubuntu16 Impala builds
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3800/1//COMMIT_MSG
Commit Message:

PS1, Line 7: CDH-41931: Toolchain for Ubuntu16 Impala builds
Is there a policy about Cloudera-specific Jira references in this code base?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 6: Code-Review+1

(1 comment)

Carrying forward.

http://gerrit.cloudera.org:8080/#/c/3800/5/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

Line 342:   packages = ["avro", "binutils", "boost", "breakpad", "bzip2", "cmake", "gcc", "gflags",
> long line
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/3800

to look at the new patch set (#6).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <cmath>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <cmath> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates impala-config.sh to add the toolchain cmake to PATH, so
that we'll use it wherever we use cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
7 files changed, 31 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/3800/6
-- 
To view, visit http://gerrit.cloudera.org:8080/3800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] CDH-41931: Toolchain for Ubuntu16 Impala builds

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: CDH-41931: Toolchain for Ubuntu16 Impala builds
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3800/1//COMMIT_MSG
Commit Message:

PS1, Line 7: CDH-41931: Toolchain for Ubuntu16 Impala builds
> Is there a policy about Cloudera-specific Jira references in this code base
Try and avoid them. Including here. Best to file a public JIRA.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 7: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 5: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3800/5/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

Line 342:   packages = ["avro", "binutils", "boost", "breakpad", "bzip2", "cmake", "gcc", "gflags", "glog",
long line


http://gerrit.cloudera.org:8080/#/c/3800/5/bin/impala-config.sh
File bin/impala-config.sh:

Line 310: export PATH=$IMPALA_HOME/bin:$IMPALA_TOOLCHAIN/cmake-$IMPALA_CMAKE_VERSION/bin/:$PATH
I'm not a big fan of dumping everything into the developer's shell, but I think that ship sailed a long time ago and this seems unlikely to have any bad consequences.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#5).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <cmath>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <cmath> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates impala-config.sh to add the toolchain cmake to PATH, so
that we'll use it wherever we use cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
7 files changed, 30 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/3800/5
-- 
To view, visit http://gerrit.cloudera.org:8080/3800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#4).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <cmath>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <cmath> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates impala-config.sh to add the toolchain cmake to PATH, so
that we'll use it wherever we use cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
7 files changed, 21 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 5: Code-Review+1

Thanks! I think it's the right thing to do, but let's get someone else to consider the change to PATH.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3800/6/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

PS6, Line 49:   "ubuntu15.10" : "ec2-package-ubuntu-14-04",
oh, also please add ubuntu16.04 -> ec2-package-ubuntu-16-04


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:

Line 18: 
> please put this above boost
Done


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value-ir.cc
File be/src/runtime/raw-value-ir.cc:

PS4, Line 15: #include "runtime/raw-value.h"
            : 
            : #include <cmath>
> This seems weird to me. Can you change this to the following and make sure 
The reason, of course, to include the ".inline" files is that it otherwise won't compile if you use any of the functions defined there. It happens to be the case here that we're not using any of the functions defined in "raw-value.inline.h", so it can be removed. (this necessitates adding some imports to this file that it was getting from there, but it still reduces the compilation dependencies a little so it seems worth it).


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value.inline.h
File be/src/runtime/raw-value.inline.h:

PS4, Line 22: 
> Put this above the boost header to be safe
Done


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:

PS4, Line 14: 
            : #include <cmath>
            : #include <gtest/gtest.h>
            : #include <boost/scoped_ptr.hpp>
> Can you clean this up while you're here, i.e. move these down with the othe
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3800/6/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:

PS6, Line 49:   "ubuntu15.10" : "ec2-package-ubuntu-14-04",
> oh, also please add ubuntu16.04 -> ec2-package-ubuntu-16-04
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] CDH-41931: Toolchain for Ubuntu16 Impala builds

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: CDH-41931: Toolchain for Ubuntu16 Impala builds
......................................................................


Patch Set 1:

(1 comment)

How about the change to use the cmake from the toolchain?

http://gerrit.cloudera.org:8080/#/c/3800/1/be/src/runtime/raw-value.inline.h
File be/src/runtime/raw-value.inline.h:

Line 22: #include <math.h>
can you move this above the boost include? I found that the order mattered in at least one case. I think it's OK to put std lib includes above other libraries. You can put an extra blank line between them to indicate its a different group.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3800/3/bin/impala-config.sh
File bin/impala-config.sh:

PS3, Line 310: export PATH=$IMPALA_HOME/bin:$PATH
Let's actually add our cmake to the path rather than using the CMAKE_CMD variable in the other file. That'd be better because it looks like we also call cmake in buildall.sh for the lzo binary.


http://gerrit.cloudera.org:8080/#/c/3800/3/bin/make_impala.sh
File bin/make_impala.sh:

PS3, Line 29: CMAKE_CMD="$IMPALA_TOOLCHAIN/cmake-$IMPALA_CMAKE_VERSION/bin/cmake"
let's remove this here and instead put this directory in the PATH, see my comment in impala-config.sh


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3800/3//COMMIT_MSG
Commit Message:

PS3, Line 11: <math.h>
I think in C++ <cmath> is the way to reference that file, and the same for cstdint and cstdio and so on. Those, as I understand it, put their identifiers in ::std::


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3800/3//COMMIT_MSG
Commit Message:

PS3, Line 11: <cmath>)
> I think in C++ <cmath> is the way to reference that file, and the same for 
Done


http://gerrit.cloudera.org:8080/#/c/3800/3/bin/impala-config.sh
File bin/impala-config.sh:

PS3, Line 310: export PATH=$IMPALA_HOME/bin:$IMPA
> Let's actually add our cmake to the path rather than using the CMAKE_CMD va
Done


http://gerrit.cloudera.org:8080/#/c/3800/3/bin/make_impala.sh
File bin/make_impala.sh:

PS3, Line 29: 
> let's remove this here and instead put this directory in the PATH, see my c
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <cmath>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <cmath> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates impala-config.sh to add the toolchain cmake to PATH, so
that we'll use it wherever we use cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Reviewed-on: http://gerrit.cloudera.org:8080/3800
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
7 files changed, 32 insertions(+), 23 deletions(-)

Approvals:
  Matthew Jacobs: Looks good to me, approved
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <math.h>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <math.h> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates make_impala.sh to use the toolchain version of cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/make_impala.sh
8 files changed, 23 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/3800/2
-- 
To view, visit http://gerrit.cloudera.org:8080/3800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 7: Code-Review+2

carrying the +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#3).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <math.h>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <math.h> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates make_impala.sh to use the toolchain version of cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/make_impala.sh
8 files changed, 24 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/3800

to look at the new patch set (#7).

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................

IMPALA-3924: Ubuntu16 support

One problem uncovered while trying to build Impala on Ubuntu16 is
that the functions 'isnan' and 'isinf' both appear in std::
(from <cmath>) and in boost::math::, but we're currently using
them without qualifiers in several places, leading to a conflict.

This patch prefaces all uses with 'std::' to disambiguate, and also
adds <cmath> imports to all files that use those functions, for
the sake of explicitness.

Another problem is that bin/make_impala.sh uses the system cmake,
which may not be compatible with the toolchain binaries. This patch
updates impala-config.sh to add the toolchain cmake to PATH, so
that we'll use it wherever we use cmake.

Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
---
M be/src/exprs/cast-functions-ir.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.inline.h
M be/src/util/metrics-test.cc
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
7 files changed, 32 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/3800/7
-- 
To view, visit http://gerrit.cloudera.org:8080/3800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-3924: Ubuntu16 support

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3924: Ubuntu16 support
......................................................................


Patch Set 4:

(4 comments)

Thanks, I think it makes sense. Just a few small cleanup things while you're touching these headers.

As a reference:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/exprs/cast-functions-ir.cc
File be/src/exprs/cast-functions-ir.cc:

Line 18: #include <cmath>
please put this above boost


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value-ir.cc
File be/src/runtime/raw-value-ir.cc:

PS4, Line 15: #include "runtime/raw-value.inline.h"
            : 
            : #include "runtime/decimal-value.inline.h"
This seems weird to me. Can you change this to the following and make sure it compiles, or maybe someone else knows why we're not pulling in the actual header directly.

 
#include "runtime/raw-value.h"


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/runtime/raw-value.inline.h
File be/src/runtime/raw-value.inline.h:

PS4, Line 22: #include <cmath>
Put this above the boost header to be safe


http://gerrit.cloudera.org:8080/#/c/3800/4/be/src/util/metrics-test.cc
File be/src/util/metrics-test.cc:

PS4, Line 14: 
            : #include "util/metrics.h"
            : #include "util/collection-metrics.h"
            : #include "util/memory-metrics.h"
Can you clean this up while you're here, i.e. move these down with the other local includes?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1520c1e4aa4175468ac342b14c1262fa745f7a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes