kou commented on code in PR #14585: URL: https://github.com/apache/arrow/pull/14585#discussion_r1034366171
########## cpp/CMakeLists.txt: ########## @@ -796,6 +805,15 @@ if(ARROW_USE_XSIMD) list(APPEND ARROW_STATIC_LINK_LIBS xsimd) endif() +if(ARROW_WITH_QPL) + add_definitions(-DENABLE_QPL_ANALYSIS) + list(APPEND ARROW_STATIC_LINK_LIBS qpl::qpl) + list(APPEND ARROW_SHARED_LINK_LIBS qpl::qpl) + if(QPL_SOURCE STREQUAL "SYSTEM") + list(APPEND ARROW_STATIC_INSTALL_INTERFACE_LIBS qpl::qpl) + endif() +endif() Review Comment: It seems that this is duplicated. ########## cpp/cmake_modules/DefineOptions.cmake: ########## @@ -375,6 +375,8 @@ takes precedence over ccache if a storage backend is configured" ON) define_option(ARROW_PLASMA "Build the plasma object store along with Arrow" OFF) + define_option(ARROW_WITH_QPL "Enable Intel® Query Processing Library" OFF) Review Comment: Could you move this to other `ARROW_WITH_XXX` options like `ARROW_WITH_UCX` and `ARROW_WITH_UTF8PROC`? ########## cpp/cmake_modules/FindQPL.cmake: ########## @@ -0,0 +1,66 @@ +# Copyright 2012 Cloudera Inc. +# +# Licensed 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. + +# - Find QPL +# +# Variables used by this module, they can change the default behaviour and need +# to be set before calling find_package: +# +# QPL_ROOT - When set, this path is inspected instead of standard library +# locations as the root of the Thrift installation. +# The environment variable THRIFT_HOME overrides this variable. +# +# This module defines +# QPL_FOUND, whether Thrift is found or not +# QPL_COMPILER_FOUND, whether Thrift compiler is found or not Review Comment: Thrift? Did you copy `FindTrhiftAlt.cmake`? ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -72,7 +72,8 @@ set(ARROW_THIRDPARTY_DEPENDENCIES utf8proc xsimd ZLIB - zstd) + zstd + QPL) Review Comment: Could you keep this list in alphabetical order? ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -2203,6 +2206,53 @@ if(ARROW_WITH_RAPIDJSON) endif() endif() + +macro(build_qpl) + message(STATUS "Building QPL from source") + set(QPL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep/src/qpl_ep-install") + if(MSVC) + set(QPL_STATIC_LIB_NAME qplstatic.lib) + else() + set(QPL_STATIC_LIB_NAME libqpl.a) + endif() + set(QPL_STATIC_LIB "${QPL_PREFIX}/lib64/${QPL_STATIC_LIB_NAME}") + set(QPL_CMAKE_ARGS + ${EP_COMMON_CMAKE_ARGS} + -DCMAKE_BUILD_TYPE=Release Review Comment: We don't need this because `EP_COMMON_CMAKE_ARGS` includes `-DCMAKE_BUILD_TYPE`. ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -2203,6 +2206,53 @@ if(ARROW_WITH_RAPIDJSON) endif() endif() + +macro(build_qpl) + message(STATUS "Building QPL from source") + set(QPL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep/src/qpl_ep-install") + if(MSVC) + set(QPL_STATIC_LIB_NAME qplstatic.lib) + else() + set(QPL_STATIC_LIB_NAME libqpl.a) + endif() Review Comment: Can we use `CMAKE_STATIC_LIBRARY_PREFIX`/`CMAKE_STATIC_LIBRARY_SUFFIX`? ```cmake set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}sql${CMAKE_STATIC_LIBRARY_SUFFIX}) ``` ########## cpp/src/arrow/util/rle_encoding.h: ########## @@ -31,6 +31,12 @@ #include "arrow/util/bit_util.h" #include "arrow/util/macros.h" +#ifdef ENABLE_QPL_ANALYSIS Review Comment: Do we need this guard? ########## cpp/CMakeLists.txt: ########## @@ -347,6 +347,12 @@ if(UNIX) add_custom_target(iwyu-all ${BUILD_SUPPORT_DIR}/iwyu/iwyu.sh all) endif(UNIX) +if(ENABLE_QPL) + add_subdirectory(build-support/qpl-cmake) + add_definitions(-DENABLE_QPL_ANALYSIS) Review Comment: Could you confirm this? ########## cpp/src/parquet/encoding.h: ########## @@ -278,7 +284,11 @@ class TypedDecoder : virtual public Decoder { /// \return The number of values decoded. Should be identical to max_values except /// at the end of the current data page. virtual int Decode(T* buffer, int max_values) = 0; - +#ifdef ENABLE_QPL_ANALYSIS + virtual int DecodeWithIAA(T* buffer, int num_values, int32_t* qpl_job_id, qpl_job** job, std::vector<uint8_t>** destination, T** out) { + return Decode(buffer, num_values); + } +#endif Review Comment: Do we still need this? ########## cpp/cmake_modules/FindQPL.cmake: ########## @@ -0,0 +1,66 @@ +# Copyright 2012 Cloudera Inc. Review Comment: Where did you copy from? ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -2203,6 +2206,53 @@ if(ARROW_WITH_RAPIDJSON) endif() endif() + +macro(build_qpl) + message(STATUS "Building QPL from source") + set(QPL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep/src/qpl_ep-install") + if(MSVC) + set(QPL_STATIC_LIB_NAME qplstatic.lib) + else() + set(QPL_STATIC_LIB_NAME libqpl.a) + endif() + set(QPL_STATIC_LIB "${QPL_PREFIX}/lib64/${QPL_STATIC_LIB_NAME}") Review Comment: `lib64` is environment dependent. We can always use `lib` with `-DCMAKE_INSTALL_LIBDIR=lib`. ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -203,6 +204,8 @@ macro(build_dependency DEPENDENCY_NAME) build_zlib() elseif("${DEPENDENCY_NAME}" STREQUAL "zstd") build_zstd() + elseif("${DEPENDENCY_NAME}" STREQUAL "QPL") + build_qpl() Review Comment: Ditto. ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -2203,6 +2206,53 @@ if(ARROW_WITH_RAPIDJSON) endif() endif() + +macro(build_qpl) + message(STATUS "Building QPL from source") + set(QPL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep/src/qpl_ep-install") + if(MSVC) + set(QPL_STATIC_LIB_NAME qplstatic.lib) + else() + set(QPL_STATIC_LIB_NAME libqpl.a) + endif() + set(QPL_STATIC_LIB "${QPL_PREFIX}/lib64/${QPL_STATIC_LIB_NAME}") + set(QPL_CMAKE_ARGS + ${EP_COMMON_CMAKE_ARGS} + -DCMAKE_BUILD_TYPE=Release + "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}" + EXCLUDE_FROM_ALL NOT) Review Comment: Why do we need this? ########## cpp/cmake_modules/FindQPL.cmake: ########## @@ -0,0 +1,66 @@ +# Copyright 2012 Cloudera Inc. +# +# Licensed 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. + +# - Find QPL +# +# Variables used by this module, they can change the default behaviour and need +# to be set before calling find_package: +# +# QPL_ROOT - When set, this path is inspected instead of standard library +# locations as the root of the Thrift installation. +# The environment variable THRIFT_HOME overrides this variable. +# +# This module defines +# QPL_FOUND, whether Thrift is found or not +# QPL_COMPILER_FOUND, whether Thrift compiler is found or not +# +# qpl::qpl, a library target to use QPL + +if(QPL_FOUND) + return() +endif() + +if(QPL_ROOT) + find_library(QPL_STATIC_LIB + NAMES qpl + PATHS ${QPL_ROOT} + NO_DEFAULT_PATH + PATH_SUFFIXES ${ARROW_LIBRARY_PATH_SUFFIXES}) + find_path(QPL_INCLUDE_DIRS + NAMES qpl.h + PATHS ${QPL_ROOT} + NO_DEFAULT_PATH + PATH_SUFFIXES ${ARROW_INCLUDE_PATH_SUFFIXES}) +else() + find_library(QPL_STATIC_LIB + NAMES qpl + PATH_SUFFIXES ${ARROW_LIBRARY_PATH_SUFFIXES}) + find_path(QPL_INCLUDE_DIRS + NAMES qpl.h + PATH_SUFFIXES ${ARROW_INCLUDE_PATH_SUFFIXES}) +endif() + +if(QPL_STATIC_LIB) + set(QPL_FOUND TRUE) + add_library(qpl::qpl STATIC IMPORTED) + set_target_properties(qpl::qpl + PROPERTIES IMPORTED_LOCATION "${QPL_STATIC_LIB}" + INTERFACE_INCLUDE_DIRECTORIES "${QPL_INCLUDE_DIRS}") +else() + if(QPL_FIND_REQUIRED) + message(FATAL_ERROR "QPL library was required in toolchain and unable to locate") + endif() + set(QPL_FOUND FALSE) +endif() Review Comment: We can use `find_package_handle_standard_args()`: https://cmake.org/cmake/help/latest/module/FindPackageHandleStandardArgs.html ########## cpp/src/arrow/CMakeLists.txt: ########## @@ -228,6 +228,7 @@ set(ARROW_SRCS util/uri.cc util/utf8.cc util/value_parsing.cc + util/qpl_job_pool.cc Review Comment: Could you keep this list in alphabetical order? ########## cpp/cmake_modules/ThirdpartyToolchain.cmake: ########## @@ -2203,6 +2206,53 @@ if(ARROW_WITH_RAPIDJSON) endif() endif() + +macro(build_qpl) + message(STATUS "Building QPL from source") + set(QPL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep/src/qpl_ep-install") + if(MSVC) + set(QPL_STATIC_LIB_NAME qplstatic.lib) + else() + set(QPL_STATIC_LIB_NAME libqpl.a) + endif() + set(QPL_STATIC_LIB "${QPL_PREFIX}/lib64/${QPL_STATIC_LIB_NAME}") + set(QPL_CMAKE_ARGS + ${EP_COMMON_CMAKE_ARGS} + -DCMAKE_BUILD_TYPE=Release + "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}" + EXCLUDE_FROM_ALL NOT) + + + ExternalProject_Add(qpl_ep + ${EP_LOG_OPTIONS} + GIT_REPOSITORY https://github.com/intel/qpl.git + GIT_TAG v0.2.1 + GIT_SUBMODULES_RECURSE TRUE Review Comment: We want to use source archive instead of Git repository to reduce Git dependency on build. See also `cpp/thirdparty/versions.txt`. ########## cpp/src/arrow/util/rle_encoding.h: ########## @@ -31,6 +31,12 @@ #include "arrow/util/bit_util.h" #include "arrow/util/macros.h" +#ifdef ENABLE_QPL_ANALYSIS +#include <qpl/qpl.hpp> +#include <qpl/qpl.h> Review Comment: Do we need them? ########## cpp/src/arrow/util/qpl_job_pool.cc: ########## @@ -0,0 +1,120 @@ +// 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. + +#include <arrow/status.h> + +#ifdef ENABLE_QPL_ANALYSIS Review Comment: We want to use `ARROW_` prefix for our macros. In general, we use `ARROW_WITH_XXX` name for this use case. ########## cpp/src/parquet/CMakeLists.txt: ########## @@ -168,6 +167,7 @@ set(PARQUET_SRCS murmur3.cc "${ARROW_SOURCE_DIR}/src/generated/parquet_constants.cpp" "${ARROW_SOURCE_DIR}/src/generated/parquet_types.cpp" + "${ARROW_SOURCE_DIR}/src/arrow/util/qpl_job_pool.cc" Review Comment: Why do we need to add this here not `cpp/src/arrow/`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
