From 28fb3f3140307970cd3f1723a9d88210458abc54 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Thu, 4 Oct 2018 17:56:38 +0530 Subject: [PATCH 01/14] ARROW-3382: [Gandiva][C++] Run tests in travis --- .travis.yml | 23 +++++++++++++++++++++++ ci/travis_before_script_cpp.sh | 4 ++++ ci/travis_script_gandiva.sh | 31 +++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100755 ci/travis_script_gandiva.sh diff --git a/.travis.yml b/.travis.yml index 09487f90f208..4c4ead6c3295 100644 --- a/.travis.yml +++ b/.travis.yml @@ -92,6 +92,29 @@ matrix: - export PLASMA_VALGRIND=1 - $TRAVIS_BUILD_DIR/ci/travis_script_python.sh 3.6 - $TRAVIS_BUILD_DIR/ci/travis_upload_cpp_coverage.sh + # Gandiva C++ w/ gcc 4.9 + - compiler: gcc + language: cpp + os: linux + jdk: openjdk8 + env: + - ARROW_TRAVIS_USE_TOOLCHAIN=1 + - ARROW_TRAVIS_VALGRIND=1 + - ARROW_TRAVIS_CLANG_FORMAT=1 + - ARROW_BUILD_WARNING_LEVEL=CHECKIN + - ARROW_TRAVIS_GANDIVA=1 + - MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9" + before_script: + # Run if something changed in CPP. + - if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi + - $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh + - $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh + - $TRAVIS_BUILD_DIR/ci/travis_lint.sh + # If either C++ or Python changed, we must install the C++ libraries + - git submodule update --init + - $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh --only-library + script: + - $TRAVIS_BUILD_DIR/ci/travis_script_gandiva_cpp.sh # [OS X] C++ & Python w/ XCode 6.4 - compiler: clang language: cpp diff --git a/ci/travis_before_script_cpp.sh b/ci/travis_before_script_cpp.sh index 1729ca4fa463..1b389a21de12 100755 --- a/ci/travis_before_script_cpp.sh +++ b/ci/travis_before_script_cpp.sh @@ -88,6 +88,10 @@ if [ $ARROW_TRAVIS_PARQUET == "1" ]; then -DPARQUET_BUILD_EXECUTABLES=ON" fi +if [ $ARROW_TRAVIS_GANDIVA == "1" ]; then + CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS -DARROW_GANDIVA=ON" +fi + if [ $ARROW_TRAVIS_VALGRIND == "1" ]; then CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS -DARROW_TEST_MEMCHECK=ON" fi diff --git a/ci/travis_script_gandiva.sh b/ci/travis_script_gandiva.sh new file mode 100755 index 000000000000..922978a3b9cd --- /dev/null +++ b/ci/travis_script_gandiva.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +set -e + +source $TRAVIS_BUILD_DIR/ci/travis_env_common.sh + +pushd $CPP_BUILD_DIR + +PATH=$ARROW_BUILD_TYPE:$PATH ctest -j2 --output-on-failure -L unittest +PATH=$ARROW_BUILD_TYPE:$PATH ctest -j2 --output-on-failure -L integ + +popd + +# TODO : Capture C++ coverage info From fccf41043ab28278d254bf59a41062637b20d927 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Thu, 4 Oct 2018 21:16:07 +0530 Subject: [PATCH 02/14] ARROW-3382: [Gandiva][C++] temp workaround re2 --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 2e2f7d1b32b5..35743b1f717c 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -37,7 +37,7 @@ if (NOT "$ENV{ARROW_BUILD_TOOLCHAIN}" STREQUAL "") # set(ORC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(PROTOBUF_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(RAPIDJSON_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") - set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") + #set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(SNAPPY_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(THRIFT_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(ZLIB_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") From 3001bc3c79663320e10fcfc5d6ac591b68bb7058 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Thu, 4 Oct 2018 11:55:40 -0400 Subject: [PATCH 03/14] Add option to dynamically-link re2, use in Gandiva build temporarily --- ci/travis_before_script_cpp.sh | 5 ++++- ci/travis_install_toolchain.sh | 3 ++- cpp/cmake_modules/FindRE2.cmake | 22 ++++++++++++++++++--- cpp/cmake_modules/GandivaBuildUtils.cmake | 2 +- cpp/cmake_modules/ThirdpartyToolchain.cmake | 21 +++++++++++++++++--- cpp/src/gandiva/CMakeLists.txt | 4 +--- 6 files changed, 45 insertions(+), 12 deletions(-) diff --git a/ci/travis_before_script_cpp.sh b/ci/travis_before_script_cpp.sh index 1b389a21de12..8e890fe62307 100755 --- a/ci/travis_before_script_cpp.sh +++ b/ci/travis_before_script_cpp.sh @@ -89,7 +89,10 @@ if [ $ARROW_TRAVIS_PARQUET == "1" ]; then fi if [ $ARROW_TRAVIS_GANDIVA == "1" ]; then - CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS -DARROW_GANDIVA=ON" + # TODO(wesm): Revert to static linkage once toolchain issues resolved + CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS \ +-DARROW_RE2_LINKAGE='shared' \ +-DARROW_GANDIVA=ON" fi if [ $ARROW_TRAVIS_VALGRIND == "1" ]; then diff --git a/ci/travis_install_toolchain.sh b/ci/travis_install_toolchain.sh index 405c496f8ce1..eb4bdf21d65c 100755 --- a/ci/travis_install_toolchain.sh +++ b/ci/travis_install_toolchain.sh @@ -40,5 +40,6 @@ if [ ! -e $CPP_TOOLCHAIN ]; then thrift-cpp=0.11.0 \ zlib \ glog \ - zstd + zstd \ + re2 fi diff --git a/cpp/cmake_modules/FindRE2.cmake b/cpp/cmake_modules/FindRE2.cmake index f2a4670c6154..42aea146f121 100644 --- a/cpp/cmake_modules/FindRE2.cmake +++ b/cpp/cmake_modules/FindRE2.cmake @@ -22,6 +22,7 @@ # This module defines # RE2_INCLUDE_DIR, directory containing headers # RE2_STATIC_LIB, path to libre2.a +# RE2_SHARED_LIB, path to libre2.so # RE2_FOUND, whether re2 has been found if( NOT "${RE2_HOME}" STREQUAL "") @@ -51,9 +52,22 @@ find_library(RE2_STATIC_LIB NAMES libre2${CMAKE_STATIC_LIBRARY_SUFFIX} DOC "Google's re2 regex static library" ) +find_library(RE2_SHARED_LIB NAMES libre2${CMAKE_SHARED_LIBRARY_SUFFIX} + PATHS ${_re2_path} + NO_DEFAULT_PATH + PATH_SUFFIXES ${lib_dirs} + DOC "Google's re2 regex static library" +) + message(STATUS ${RE2_INCLUDE_DIR}) -if (NOT RE2_INCLUDE_DIR OR NOT RE2_STATIC_LIB) +if (ARROW_RE2_LINKAGE STREQUAL "static" AND (NOT RE2_STATIC_LIB)) + set(RE2_LIB_NOT_FOUND TRUE) +elseif(ARROW_RE2_LINKAGE STREQUAL "shared" AND (NOT RE2_SHARED_LIB)) + set(RE2_LIB_NOT_FOUND TRUE) +endif() + +if (NOT RE2_INCLUDE_DIR OR RE2_LIB_NOT_FOUND) set(RE2_FOUND FALSE) if (_re2_path) set (RE2_ERR_MSG "Could not find re2. Looked in ${_re2_path}.") @@ -68,11 +82,13 @@ if (NOT RE2_INCLUDE_DIR OR NOT RE2_STATIC_LIB) endif () else() set(RE2_FOUND TRUE) - message(STATUS "Found the RE2 headers : ${RE2_INCLUDE_DIR}") - message(STATUS "Found the RE2 static library : ${RE2_STATIC_LIB}") + message(STATUS "RE2 headers : ${RE2_INCLUDE_DIR}") + message(STATUS "RE2 static library : ${RE2_STATIC_LIB}") + message(STATUS "RE2 shared library : ${RE2_SHARED_LIB}") endif() mark_as_advanced( RE2_INCLUDE_DIR + RE2_SHARED_LIB RE2_STATIC_LIB ) diff --git a/cpp/cmake_modules/GandivaBuildUtils.cmake b/cpp/cmake_modules/GandivaBuildUtils.cmake index a1ed48eee834..ff6fcc2d2734 100644 --- a/cpp/cmake_modules/GandivaBuildUtils.cmake +++ b/cpp/cmake_modules/GandivaBuildUtils.cmake @@ -39,7 +39,7 @@ function(build_gandiva_lib TYPE ARROW) Boost::system Boost::filesystem LLVM::LLVM_INTERFACE - ${RE2_STATIC_LIB}) + re2) if (${TYPE} MATCHES "static" AND NOT APPLE) target_link_libraries(gandiva_${TYPE} diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 35743b1f717c..937e8c5d4bc5 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -15,6 +15,11 @@ # specific language governing permissions and limitations # under the License. +# ---------------------------------------------------------------------- +# Toolchain linkage options + +set(ARROW_RE2_LINKAGE "static" CACHE STRING + "How to link the re2 library. static|shared (default shared)") # ---------------------------------------------------------------------- # Thirdparty versions, environment variables, source URLs @@ -37,7 +42,7 @@ if (NOT "$ENV{ARROW_BUILD_TOOLCHAIN}" STREQUAL "") # set(ORC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(PROTOBUF_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(RAPIDJSON_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") - #set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") + set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(SNAPPY_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(THRIFT_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(ZLIB_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") @@ -100,6 +105,10 @@ if (DEFINED ENV{RAPIDJSON_HOME}) set(RAPIDJSON_HOME "$ENV{RAPIDJSON_HOME}") endif() +if (DEFINED ENV{RE2_HOME}) + set(RE2_HOME "$ENV{RAPIDJSON_HOME}") +endif() + if (DEFINED ENV{SNAPPY_HOME}) set(SNAPPY_HOME "$ENV{SNAPPY_HOME}") endif() @@ -1091,8 +1100,14 @@ if (ARROW_GANDIVA) endif () include_directories (SYSTEM ${RE2_INCLUDE_DIR}) - ADD_THIRDPARTY_LIB(re2 - STATIC_LIB ${RE2_STATIC_LIB}) + + if (ARROW_RE2_LINKAGE STREQUAL "shared") + ADD_THIRDPARTY_LIB(re2 + STATIC_LIB ${RE2_SHARED_LIB}) + else() + ADD_THIRDPARTY_LIB(re2 + STATIC_LIB ${RE2_STATIC_LIB}) + endif() if (RE2_VENDORED) add_dependencies (arrow_dependencies re2_ep) diff --git a/cpp/src/gandiva/CMakeLists.txt b/cpp/src/gandiva/CMakeLists.txt index 91f851fb92b6..d47a685cbbc6 100644 --- a/cpp/src/gandiva/CMakeLists.txt +++ b/cpp/src/gandiva/CMakeLists.txt @@ -20,8 +20,6 @@ cmake_minimum_required(VERSION 3.11) project(gandiva) -find_package(RE2 REQUIRED) - include(GandivaBuildUtils) find_package(LLVM) @@ -120,7 +118,7 @@ target_include_directories(gandiva_helpers ${ARROW_INCLUDE_DIR} ) -target_link_libraries(gandiva_helpers PRIVATE Boost::boost ${RE2_STATIC_LIB}) +target_link_libraries(gandiva_helpers PRIVATE Boost::boost re2) if (NOT APPLE) target_link_libraries(gandiva_helpers LINK_PRIVATE -static-libstdc++ -static-libgcc) endif() From 8f881d5f01df8f60518fdea4d886d71eff2becea Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Fri, 5 Oct 2018 16:59:59 +0530 Subject: [PATCH 04/14] ARROW-3382: fix compiler warnings --- cpp/src/arrow/util/bit-util.h | 9 +++++++++ cpp/src/gandiva/bitmap_accumulator_test.cc | 4 ++-- cpp/src/gandiva/precompiled/CMakeLists.txt | 2 +- cpp/src/gandiva/precompiled/arithmetic_ops.cc | 4 +++- cpp/src/gandiva/precompiled/bitmap.cc | 20 ++++++++----------- cpp/src/gandiva/precompiled/hash_test.cc | 8 ++++---- cpp/src/gandiva/tests/projector_test.cc | 8 ++++---- 7 files changed, 31 insertions(+), 24 deletions(-) diff --git a/cpp/src/arrow/util/bit-util.h b/cpp/src/arrow/util/bit-util.h index a22ff0fc4b92..4bb44111f80d 100644 --- a/cpp/src/arrow/util/bit-util.h +++ b/cpp/src/arrow/util/bit-util.h @@ -325,6 +325,15 @@ static inline void ClearBit(uint8_t* bits, int64_t i) { static inline void SetBit(uint8_t* bits, int64_t i) { bits[i / 8] |= kBitmask[i % 8]; } +static inline void SetBitTo(uint8_t* bits, int64_t i, bool bit_is_set) { + // https://graphics.stanford.edu/~seander/bithacks.html + // "Conditionally set or clear bits without branching" + // NOTE: this seems to confuse Valgrind as it reads from potentially + // uninitialized memory + bits[i / 8] ^= static_cast(-static_cast(bit_is_set) ^ bits[i / 8]) & + kBitmask[i % 8]; +} + /// \brief Convert vector of bytes to bitmap buffer ARROW_EXPORT Status BytesToBits(const std::vector&, MemoryPool*, std::shared_ptr*); diff --git a/cpp/src/gandiva/bitmap_accumulator_test.cc b/cpp/src/gandiva/bitmap_accumulator_test.cc index c7a0b46a73af..fc89421344e8 100644 --- a/cpp/src/gandiva/bitmap_accumulator_test.cc +++ b/cpp/src/gandiva/bitmap_accumulator_test.cc @@ -38,7 +38,7 @@ void TestBitMapAccumulator::FillBitMap(uint8_t* bmap, int nrecords) { for (int i = 0; i < nbytes; ++i) { rand_r(&cur); - bmap[i] = cur % UINT8_MAX; + bmap[i] = static_cast(cur % UINT8_MAX); } } @@ -49,7 +49,7 @@ void TestBitMapAccumulator::ByteWiseIntersectBitMaps(uint8_t* dst, for (int i = 0; i < nbytes; ++i) { dst[i] = 0xff; for (uint32_t j = 0; j < srcs.size(); ++j) { - dst[i] &= srcs[j][i]; + dst[i] = dst[i] & srcs[j][i]; } } } diff --git a/cpp/src/gandiva/precompiled/CMakeLists.txt b/cpp/src/gandiva/precompiled/CMakeLists.txt index b71cde3937d4..4857314ce091 100644 --- a/cpp/src/gandiva/precompiled/CMakeLists.txt +++ b/cpp/src/gandiva/precompiled/CMakeLists.txt @@ -34,7 +34,7 @@ foreach(SRC_FILE ${PRECOMPILED_SRCS}) set(BC_FILE ${CMAKE_CURRENT_BINARY_DIR}/${SRC_BASE}.bc) add_custom_command( OUTPUT ${BC_FILE} - COMMAND ${CLANG_EXECUTABLE} + COMMAND ${CLANG_EXECUTABLE} -I${CMAKE_SOURCE_DIR}/src -std=c++11 -emit-llvm -O2 -c ${ABSOLUTE_SRC} -o ${BC_FILE} DEPENDS ${SRC_FILE}) list(APPEND BC_FILES ${BC_FILE}) diff --git a/cpp/src/gandiva/precompiled/arithmetic_ops.cc b/cpp/src/gandiva/precompiled/arithmetic_ops.cc index 20d2c69c9c0b..ebe7d59a33ba 100644 --- a/cpp/src/gandiva/precompiled/arithmetic_ops.cc +++ b/cpp/src/gandiva/precompiled/arithmetic_ops.cc @@ -57,7 +57,9 @@ extern "C" { // Symmetric binary fns : left, right params and return type are same. #define BINARY_SYMMETRIC(NAME, TYPE, OP) \ FORCE_INLINE \ - TYPE NAME##_##TYPE##_##TYPE(TYPE left, TYPE right) { return left OP right; } + TYPE NAME##_##TYPE##_##TYPE(TYPE left, TYPE right) { \ + return static_cast(left OP right); \ + } NUMERIC_TYPES(BINARY_SYMMETRIC, add, +) NUMERIC_TYPES(BINARY_SYMMETRIC, subtract, -) diff --git a/cpp/src/gandiva/precompiled/bitmap.cc b/cpp/src/gandiva/precompiled/bitmap.cc index 53f17354b36e..651f2cbf0a20 100644 --- a/cpp/src/gandiva/precompiled/bitmap.cc +++ b/cpp/src/gandiva/precompiled/bitmap.cc @@ -17,6 +17,8 @@ // BitMap functions +#include "arrow/util/bit-util.h" + extern "C" { #include "./types.h" @@ -28,26 +30,20 @@ extern "C" { #define POS_TO_BIT_INDEX(p) (p % 8) FORCE_INLINE -bool bitMapGetBit(const unsigned char* bmap, int position) { - int byteIdx = POS_TO_BYTE_INDEX(position); - int bitIdx = POS_TO_BIT_INDEX(position); - return ((bmap[byteIdx] & (1 << bitIdx)) > 0); +bool bitMapGetBit(const uint8_t* bmap, int position) { + return arrow::BitUtil::GetBit(bmap, position); } FORCE_INLINE -void bitMapSetBit(unsigned char* bmap, int position, bool value) { - int byteIdx = POS_TO_BYTE_INDEX(position); - int bitIdx = POS_TO_BIT_INDEX(position); - bmap[byteIdx] ^= (-value ^ bmap[byteIdx]) & (1UL << bitIdx); +void bitMapSetBit(uint8_t* bmap, int position, bool value) { + arrow::BitUtil::SetBitTo(bmap, position, value); } // Clear the bit if value = false. Does nothing if value = true. FORCE_INLINE -void bitMapClearBitIfFalse(unsigned char* bmap, int position, bool value) { +void bitMapClearBitIfFalse(uint8_t* bmap, int position, bool value) { if (!value) { - int byteIdx = POS_TO_BYTE_INDEX(position); - int bitIdx = POS_TO_BIT_INDEX(position); - bmap[byteIdx] &= ~(1 << bitIdx); + arrow::BitUtil::ClearBit(bmap, position); } } diff --git a/cpp/src/gandiva/precompiled/hash_test.cc b/cpp/src/gandiva/precompiled/hash_test.cc index d58344d32965..265385be85bc 100644 --- a/cpp/src/gandiva/precompiled/hash_test.cc +++ b/cpp/src/gandiva/precompiled/hash_test.cc @@ -44,8 +44,8 @@ TEST(TestHash, TestHash32) { EXPECT_EQ(hash32(u16, 0), zero_hash); EXPECT_EQ(hash32(s32, 0), zero_hash); EXPECT_EQ(hash32(u32, 0), zero_hash); - EXPECT_EQ(hash32(s64, 0), zero_hash); - EXPECT_EQ(hash32(u64, 0), zero_hash); + EXPECT_EQ(hash32(static_cast(s64), 0), zero_hash); + EXPECT_EQ(hash32(static_cast(u64), 0), zero_hash); EXPECT_EQ(hash32(f32, 0), zero_hash); EXPECT_EQ(hash32(f64, 0), zero_hash); @@ -82,8 +82,8 @@ TEST(TestHash, TestHash64) { EXPECT_EQ(hash64(u16, 0), zero_hash); EXPECT_EQ(hash64(s32, 0), zero_hash); EXPECT_EQ(hash64(u32, 0), zero_hash); - EXPECT_EQ(hash64(s64, 0), zero_hash); - EXPECT_EQ(hash64(u64, 0), zero_hash); + EXPECT_EQ(hash64(static_cast(s64), 0), zero_hash); + EXPECT_EQ(hash64(static_cast(u64), 0), zero_hash); EXPECT_EQ(hash64(f32, 0), zero_hash); EXPECT_EQ(hash64(f64, 0), zero_hash); diff --git a/cpp/src/gandiva/tests/projector_test.cc b/cpp/src/gandiva/tests/projector_test.cc index 6f72a03c81ae..57497cd1c31b 100644 --- a/cpp/src/gandiva/tests/projector_test.cc +++ b/cpp/src/gandiva/tests/projector_test.cc @@ -209,10 +209,10 @@ static void TestArithmeticOpsForType(arrow::MemoryPool* pool) { std::vector eq; std::vector lt; for (int i = 0; i < num_records; i++) { - sum.push_back(input0[i] + input1[i]); - sub.push_back(input0[i] - input1[i]); - mul.push_back(input0[i] * input1[i]); - div.push_back(input0[i] / input1[i]); + sum.push_back(static_cast(input0[i] + input1[i])); + sub.push_back(static_cast(input0[i] - input1[i])); + mul.push_back(static_cast(input0[i] * input1[i])); + div.push_back(static_cast(input0[i] / input1[i])); eq.push_back(input0[i] == input1[i]); lt.push_back(input0[i] < input1[i]); } From c594e647dfa562513cca68ad5ac876dc5c8b9918 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Fri, 5 Oct 2018 17:16:41 +0530 Subject: [PATCH 05/14] ARROW-3382: [Gandiva][C++] fix format error --- cpp/src/gandiva/precompiled/arithmetic_ops.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/gandiva/precompiled/arithmetic_ops.cc b/cpp/src/gandiva/precompiled/arithmetic_ops.cc index ebe7d59a33ba..7d05699a4ada 100644 --- a/cpp/src/gandiva/precompiled/arithmetic_ops.cc +++ b/cpp/src/gandiva/precompiled/arithmetic_ops.cc @@ -55,10 +55,10 @@ extern "C" { } // Symmetric binary fns : left, right params and return type are same. -#define BINARY_SYMMETRIC(NAME, TYPE, OP) \ - FORCE_INLINE \ +#define BINARY_SYMMETRIC(NAME, TYPE, OP) \ + FORCE_INLINE \ TYPE NAME##_##TYPE##_##TYPE(TYPE left, TYPE right) { \ - return static_cast(left OP right); \ + return static_cast(left OP right); \ } NUMERIC_TYPES(BINARY_SYMMETRIC, add, +) From c7ad2a5120e515bb675cbcd106975357fca7260c Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Fri, 5 Oct 2018 17:43:03 +0530 Subject: [PATCH 06/14] ARROW-3382: [Gandiva][CPP] fix compiler warnings --- cpp/src/gandiva/function_registry.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/gandiva/function_registry.cc b/cpp/src/gandiva/function_registry.cc index 2b7962477bb2..84a265f45f3f 100644 --- a/cpp/src/gandiva/function_registry.cc +++ b/cpp/src/gandiva/function_registry.cc @@ -365,7 +365,7 @@ FunctionRegistry::SignatureMap FunctionRegistry::pc_registry_map_ = InitPCMap(); FunctionRegistry::SignatureMap FunctionRegistry::InitPCMap() { SignatureMap map; - int num_entries = sizeof(pc_registry_) / sizeof(NativeFunction); + int num_entries = static_cast(sizeof(pc_registry_) / sizeof(NativeFunction)); printf("Registry has %d pre-compiled functions\n", num_entries); for (int i = 0; i < num_entries; i++) { From 714502dc7431888189ddb0a9988be289ed3a67d9 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Fri, 5 Oct 2018 20:27:23 +0530 Subject: [PATCH 07/14] ARROW-3382: [Gandiva][C++] switch to static RE2 --- ci/travis_before_script_cpp.sh | 5 +---- cpp/cmake_modules/ThirdpartyToolchain.cmake | 5 +++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/ci/travis_before_script_cpp.sh b/ci/travis_before_script_cpp.sh index 8e890fe62307..1b389a21de12 100755 --- a/ci/travis_before_script_cpp.sh +++ b/ci/travis_before_script_cpp.sh @@ -89,10 +89,7 @@ if [ $ARROW_TRAVIS_PARQUET == "1" ]; then fi if [ $ARROW_TRAVIS_GANDIVA == "1" ]; then - # TODO(wesm): Revert to static linkage once toolchain issues resolved - CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS \ --DARROW_RE2_LINKAGE='shared' \ --DARROW_GANDIVA=ON" + CMAKE_COMMON_FLAGS="$CMAKE_COMMON_FLAGS -DARROW_GANDIVA=ON" fi if [ $ARROW_TRAVIS_VALGRIND == "1" ]; then diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 937e8c5d4bc5..e6c7f7828f47 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -19,7 +19,7 @@ # Toolchain linkage options set(ARROW_RE2_LINKAGE "static" CACHE STRING - "How to link the re2 library. static|shared (default shared)") + "How to link the re2 library. static|shared (default static)") # ---------------------------------------------------------------------- # Thirdparty versions, environment variables, source URLs @@ -42,7 +42,8 @@ if (NOT "$ENV{ARROW_BUILD_TOOLCHAIN}" STREQUAL "") # set(ORC_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(PROTOBUF_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(RAPIDJSON_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") - set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") + # conda-forge doesn't have a static re2. + #set(RE2_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(SNAPPY_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(THRIFT_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") set(ZLIB_HOME "$ENV{ARROW_BUILD_TOOLCHAIN}") From c80b8c62a1c0a8715ee5abf01f0069034a7a77b5 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Fri, 5 Oct 2018 21:53:40 +0530 Subject: [PATCH 08/14] ARROW-3382: [Gandiva][CPP] add dep for precompiled --- cpp/cmake_modules/GandivaBuildUtils.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/cmake_modules/GandivaBuildUtils.cmake b/cpp/cmake_modules/GandivaBuildUtils.cmake index ff6fcc2d2734..4a069f0bb178 100644 --- a/cpp/cmake_modules/GandivaBuildUtils.cmake +++ b/cpp/cmake_modules/GandivaBuildUtils.cmake @@ -102,6 +102,8 @@ function(add_precompiled_unit_test REL_TEST_NAME) get_filename_component(TEST_NAME ${REL_TEST_NAME} NAME_WE) add_executable(${TEST_NAME} ${REL_TEST_NAME} ${ARGN}) + # Require toolchain to be built + add_dependencies(${TEST_NAME} arrow_dependencies) target_include_directories(${TEST_NAME} PRIVATE ${CMAKE_SOURCE_DIR}/src) target_link_libraries(${TEST_NAME} PRIVATE ${GANDIVA_TEST_LINK_LIBS}) target_compile_definitions(${TEST_NAME} PRIVATE GANDIVA_UNIT_TEST=1) From 512b7e0cf3e8cd05938b946df5f4561a3d3327e3 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 11:50:34 +0530 Subject: [PATCH 09/14] ARROW-3382: [Gandiva][C++] use gcc 4.9 --- .travis.yml | 2 ++ ci/travis_install_linux.sh | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/.travis.yml b/.travis.yml index 4c4ead6c3295..5b9990a1d5ba 100644 --- a/.travis.yml +++ b/.travis.yml @@ -104,6 +104,8 @@ matrix: - ARROW_BUILD_WARNING_LEVEL=CHECKIN - ARROW_TRAVIS_GANDIVA=1 - MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9" + before_install: + - eval "${MATRIX_EVAL}" before_script: # Run if something changed in CPP. - if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi diff --git a/ci/travis_install_linux.sh b/ci/travis_install_linux.sh index e8cd0102350f..c1db2d5c5bda 100755 --- a/ci/travis_install_linux.sh +++ b/ci/travis_install_linux.sh @@ -21,6 +21,10 @@ sudo apt-get install -y -q \ gdb binutils ccache libboost-dev libboost-filesystem-dev \ libboost-system-dev libboost-regex-dev libjemalloc-dev +if [ "$CXX" == "g++-4.9" ]; then + sudo apt-get install -y -q g++-4.9 +fi + if [ "$ARROW_TRAVIS_VALGRIND" == "1" ]; then sudo apt-get install -y -q valgrind fi From d7da6107b52382303290ba13aa5ec1b4650c1239 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 12:02:51 +0530 Subject: [PATCH 10/14] ARROW-3382: [Gandiva][C++] set C++ to g++4.9 --- .travis.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5b9990a1d5ba..9d7d482c6024 100644 --- a/.travis.yml +++ b/.travis.yml @@ -103,9 +103,8 @@ matrix: - ARROW_TRAVIS_CLANG_FORMAT=1 - ARROW_BUILD_WARNING_LEVEL=CHECKIN - ARROW_TRAVIS_GANDIVA=1 - - MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9" - before_install: - - eval "${MATRIX_EVAL}" + - CC="gcc-4.9" + - CXX="g++-4.9" before_script: # Run if something changed in CPP. - if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi From b011c644538422534a276c94a8d34fc3e4ff0457 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 12:13:40 +0530 Subject: [PATCH 11/14] ARROW-3382: [Gandiva][C++] Set gcc to 4.9 --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9d7d482c6024..675e1b302ff2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -33,6 +33,7 @@ git: before_install: # Common pre-install steps for all builds + - eval "${MATRIX_EVAL}" - ulimit -c unlimited -S - | if [ $TRAVIS_OS_NAME == "linux" ]; then @@ -103,8 +104,7 @@ matrix: - ARROW_TRAVIS_CLANG_FORMAT=1 - ARROW_BUILD_WARNING_LEVEL=CHECKIN - ARROW_TRAVIS_GANDIVA=1 - - CC="gcc-4.9" - - CXX="g++-4.9" + - MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9" before_script: # Run if something changed in CPP. - if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi From 5e9424e519653dc1124d1b904703efb5a0b14083 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 12:33:31 +0530 Subject: [PATCH 12/14] ARROW-3382: [Gandiva][C++] fix a typo in travis.yml --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 675e1b302ff2..24fde86ab057 100644 --- a/.travis.yml +++ b/.travis.yml @@ -115,7 +115,7 @@ matrix: - git submodule update --init - $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh --only-library script: - - $TRAVIS_BUILD_DIR/ci/travis_script_gandiva_cpp.sh + - $TRAVIS_BUILD_DIR/ci/travis_script_gandiva.sh # [OS X] C++ & Python w/ XCode 6.4 - compiler: clang language: cpp From 77097deafd3d8bf42fb65b310c3017cc81f17bf6 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 13:36:15 +0530 Subject: [PATCH 13/14] ARROW-3382: [Gandiva][C++] run integ tests serially --- ci/travis_script_gandiva.sh | 4 +++- cpp/src/gandiva/tests/micro_benchmarks.cc | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ci/travis_script_gandiva.sh b/ci/travis_script_gandiva.sh index 922978a3b9cd..3dbd993c0a3e 100755 --- a/ci/travis_script_gandiva.sh +++ b/ci/travis_script_gandiva.sh @@ -24,7 +24,9 @@ source $TRAVIS_BUILD_DIR/ci/travis_env_common.sh pushd $CPP_BUILD_DIR PATH=$ARROW_BUILD_TYPE:$PATH ctest -j2 --output-on-failure -L unittest -PATH=$ARROW_BUILD_TYPE:$PATH ctest -j2 --output-on-failure -L integ + +# not running in parallel, since some of them are benchmarks +PATH=$ARROW_BUILD_TYPE:$PATH ctest -VV -L integ popd diff --git a/cpp/src/gandiva/tests/micro_benchmarks.cc b/cpp/src/gandiva/tests/micro_benchmarks.cc index d8706fa09435..1a1390474f5a 100644 --- a/cpp/src/gandiva/tests/micro_benchmarks.cc +++ b/cpp/src/gandiva/tests/micro_benchmarks.cc @@ -30,7 +30,9 @@ using arrow::int32; using arrow::int64; using arrow::utf8; -float tolerance_ratio = 4.0; +// TODO ravindra: the base numbers are from a mac. they need to be caliberated +// for the hardware used by travis. +float tolerance_ratio = 6.0; class TestBenchmarks : public ::testing::Test { public: From a312b3c9243380080d7fc78df949f768e4e76777 Mon Sep 17 00:00:00 2001 From: Pindikura Ravindra Date: Sat, 6 Oct 2018 15:25:42 +0530 Subject: [PATCH 14/14] ARROW-3382: [Gandiva][C++] minor change in comment --- cpp/src/gandiva/tests/micro_benchmarks.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/gandiva/tests/micro_benchmarks.cc b/cpp/src/gandiva/tests/micro_benchmarks.cc index 1a1390474f5a..fd6d87de6f0a 100644 --- a/cpp/src/gandiva/tests/micro_benchmarks.cc +++ b/cpp/src/gandiva/tests/micro_benchmarks.cc @@ -30,7 +30,7 @@ using arrow::int32; using arrow::int64; using arrow::utf8; -// TODO ravindra: the base numbers are from a mac. they need to be caliberated +// TODO : the base numbers are from a mac. they need to be caliberated // for the hardware used by travis. float tolerance_ratio = 6.0;