You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/04 02:48:17 UTC
[GitHub] [arrow] yaqi-zhao opened a new pull request, #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
yaqi-zhao opened a new pull request, #14585:
URL: https://github.com/apache/arrow/pull/14585
Intel® In-Memory Analytics Accelerator (Intel® IAA) is a hardware accelerator available in the upcoming generation of Intel® Xeon® Scalable processors ("Sapphire Rapids"). Its goal is to speed up common operations in analytics like data (de)compression and filtering. It support decoding of Parquet RLE format. We add new codec which utilizes the Intel® IAA offloading technology to provide a high-performance RLE decode implementation. The codec uses the [Intel® Query Processing Library (QPL)](https://github.com/intel/qpl) which abstracts access to the hardware accelerator. The new solution provides in general higher performance against current solution, and also consume less CPU.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wgtmac commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
wgtmac commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1037901735
##########
cpp/cmake_modules/FindQPL.cmake:
##########
@@ -0,0 +1,54 @@
+# 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
Review Comment:
The license header looks incomplete.
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -190,6 +195,14 @@ class BitReader {
/// Maximum byte length of a vlq encoded int64
static constexpr int kMaxVlqByteLengthForInt64 = 10;
+ const uint8_t* getBuffer() { return buffer_ - 1; }
Review Comment:
Use buffer() or GetBuffer() ?
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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 ARROW_WITH_QPL
+#include "arrow/util/qpl_job_pool.h"
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::instance() {
+ static QplJobHWPool pool;
Review Comment:
Can we create it lazily?
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -200,6 +213,7 @@ class BitReader {
int byte_offset_; // Offset in buffer_
int bit_offset_; // Offset in buffered_values_
+ int val_offset_; // index of value to be read
Review Comment:
```suggestion
int read_offset_; // Index of next value to read
```
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2206,42 @@ if(ARROW_WITH_RAPIDJSON)
endif()
endif()
+macro(build_qpl)
Review Comment:
Is it possible to add the dependency to `cpp/thirdparty/versions.txt`? It will be a lot easier for developers if offline build is supported.
##########
cpp/src/parquet/encoding.cc:
##########
@@ -1493,9 +1494,24 @@ class DictDecoderImpl : public DecoderImpl, virtual public DictDecoder<Type> {
int Decode(T* buffer, int num_values) override {
num_values = std::min(num_values, num_values_);
- int decoded_values =
+ int decoded_values;
+
+#ifdef ARROW_WITH_QPL
Review Comment:
Is it possible to wrap the dispatch logic within idx_decoder_?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- closed pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
URL: https://github.com/apache/arrow/pull/14585
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1035301666
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -183,7 +184,9 @@ macro(build_dependency DEPENDENCY_NAME)
build_orc()
elseif("${DEPENDENCY_NAME}" STREQUAL "Protobuf")
build_protobuf()
- elseif("${DEPENDENCY_NAME}" STREQUAL "RapidJSON")
+ elseif("${DEPENDENCY_NAME}" STREQUAL "QPL")
+ build_qpl()
+ elseif("${DEPENDENCY_NAME}" STREQUAL "RapidJSON")
Review Comment:
```suggestion
elseif("${DEPENDENCY_NAME}" STREQUAL "RapidJSON")
```
##########
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:
When the next release is scheduled?
BTW, we can apply a patch with `externalproject_add()`: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L4104-L4113
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034402771
##########
cpp/cmake_modules/FindQPL.cmake:
##########
@@ -0,0 +1,66 @@
+# Copyright 2012 Cloudera Inc.
Review Comment:
I copied from findThrift.cmake. I'll remove this line.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1041976582
##########
cpp/src/arrow/util/qpl_job_pool.h:
##########
@@ -0,0 +1,68 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <random>
+#include <utility>
+#include <vector>
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+#include "qpl/qpl.h"
+#include "qpl/qpl.hpp"
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+/// QplJobHWPool is resource pool to provide the job objects.
+/// Job object is used for storing context information during
+// offloading compression job to HW Accelerator.
+class QplJobHWPool {
+ public:
+ QplJobHWPool();
+ ~QplJobHWPool();
+
+ static QplJobHWPool& instance();
Review Comment:
@wgtmac I have addressed your comment including function name styles, comments, and static functions. Can you please take a look again? Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045377370
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -64,6 +64,7 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
ORC
re2
Protobuf
+ QPL
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048109312
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
It will not touch invlalid memory since in function [DictDecoderImpl::SetData](https://github.com/apache/arrow/blob/f6685371032e933e31d7ae2b156a6385c00d1673/cpp/src/parquet/encoding.cc#L1491), bit reader buffer was set to <encoded-data> and skip <bit_width>
IAA need a full parquet rle buffer so I `-1` here
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
It will not touch invlalid memory since in function [DictDecoderImpl::SetData](https://github.com/apache/arrow/blob/f6685371032e933e31d7ae2b156a6385c00d1673/cpp/src/parquet/encoding.cc#L1491), bit reader buffer was set to encoded-data and skip <bit_width>
IAA need a full parquet rle buffer so I `-1` here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034435054
##########
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:
no need for this, i'll remove it
##########
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:
no need for this, i'll remove it
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034406781
##########
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:
Yes, I'll correct it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1324670051
@kou May I draw your attention to this PR.
We use IAA to do the RLE Decode work and benchmark results on ReadRowGroups API show that there are up to 2X perf improvements
Do you think this work valuable?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maqister commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
maqister commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1321374030
"The new solution provides in general higher performance against current solution, and also consume less CPU."
Could you share performance numbers? This description is very vague..
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1302887826
https://issues.apache.org/jira/browse/ARROW-17884
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045619729
##########
cpp/CMakeLists.txt:
##########
@@ -796,6 +796,15 @@ if(ARROW_USE_XSIMD)
list(APPEND ARROW_STATIC_LINK_LIBS xsimd)
endif()
+if(ARROW_WITH_QPL)
+ add_definitions(-DARROW_WITH_QPL)
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1039082153
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2206,42 @@ if(ARROW_WITH_RAPIDJSON)
endif()
endif()
+macro(build_qpl)
Review Comment:
Yes, i'll update it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048117411
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
Or is "bit-width" included in general RLE format?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048139329
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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/util/qpl_job_pool.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::GetInstance() {
+ static QplJobHWPool pool;
+ return pool;
+}
+
+QplJobHWPool::QplJobHWPool()
+ : random_engine(std::random_device()()), distribution(0, MAX_JOB_NUMBER - 1) {
+ (void)AllocateQPLJob();
+}
+
+QplJobHWPool::~QplJobHWPool() {
+ for (uint32_t i = 0; i < MAX_JOB_NUMBER; ++i) {
+ if (hw_job_ptr_pool[i]) {
+ qpl_fini_job(hw_job_ptr_pool[i]);
+ hw_job_ptr_pool[i] = nullptr;
+ }
+ }
+ iaa_job_ready = false;
+}
+
+arrow::Status QplJobHWPool::AllocateQPLJob() {
+ uint32_t job_size = 0;
+
+ /// Get size required for saving a single qpl job object
+ qpl_get_job_size(qpl_path_hardware, &job_size);
+ /// Allocate entire buffer for storing all job objects
+ hw_jobs_buffer = std::make_unique<uint8_t[]>(job_size * MAX_JOB_NUMBER);
+ /// Initialize pool for storing all job object pointers
+ /// Reallocate buffer by shifting address offset for each job object.
+ for (uint32_t index = 0; index < MAX_JOB_NUMBER; ++index) {
+ qpl_job* qpl_job_ptr =
+ reinterpret_cast<qpl_job*>(hw_jobs_buffer.get() + index * job_size);
+ if (qpl_init_job(qpl_path_hardware, qpl_job_ptr) != QPL_STS_OK) {
+ iaa_job_ready = false;
+ return arrow::Status::Invalid(
+ "Initialization of hardware IAA failed."
+ " Please check if Intel In-Memory Analytics Accelerator (IAA) "
+ "is properly set up!");
+ }
+ hw_job_ptr_pool[index] = qpl_job_ptr;
+ job_ptr_locks[index].store(false);
+ }
+
+ iaa_job_ready = true;
+ return arrow::Status::OK();
+}
+
+qpl_job* QplJobHWPool::AcquireJob(uint32_t& job_id) {
+ if (!job_ready()) {
+ return nullptr;
+ }
+ uint32_t retry = 0;
+ auto index = distribution(random_engine);
Review Comment:
Our initial idea is to avoid finding available job from the first index and want to save time.
we choose Mersenne twister as random engine and std::uniform_int_distribution as distribution to guarantee the randomness.
I'm not sure if there is more efficient way, do you have some suggestions?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1351079637
I haven't looked at this in detail but my general sentiment is negative.
This PR does seem to provide very significant speedups. However, it comes with several downsides:
* the accelerator is Intel-specific
* it will only be provided on high-end CPUs (Xeon Scalable), and maybe even only some of them due to market segmentation
* it requires a dedicated third-party library for which we have to maintain vendoring support
Generally, our Arrow and Parquet C++ maintenance bandwidth is very limited, with few active maintainers. This PR adds maintenance overhead for no value to most users.
If Intel were a significant contributor to Arrow and Parquet maintenance, I would view this PR (and other similar one-shot PRs that add accelerations to select parts of the codebase) more favorably.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048070297
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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/util/qpl_job_pool.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::GetInstance() {
+ static QplJobHWPool pool;
+ return pool;
+}
+
+QplJobHWPool::QplJobHWPool()
+ : random_engine(std::random_device()()), distribution(0, MAX_JOB_NUMBER - 1) {
+ (void)AllocateQPLJob();
+}
+
+QplJobHWPool::~QplJobHWPool() {
+ for (uint32_t i = 0; i < MAX_JOB_NUMBER; ++i) {
+ if (hw_job_ptr_pool[i]) {
+ qpl_fini_job(hw_job_ptr_pool[i]);
+ hw_job_ptr_pool[i] = nullptr;
+ }
+ }
+ iaa_job_ready = false;
+}
+
+arrow::Status QplJobHWPool::AllocateQPLJob() {
+ uint32_t job_size = 0;
+
+ /// Get size required for saving a single qpl job object
+ qpl_get_job_size(qpl_path_hardware, &job_size);
+ /// Allocate entire buffer for storing all job objects
+ hw_jobs_buffer = std::make_unique<uint8_t[]>(job_size * MAX_JOB_NUMBER);
+ /// Initialize pool for storing all job object pointers
+ /// Reallocate buffer by shifting address offset for each job object.
+ for (uint32_t index = 0; index < MAX_JOB_NUMBER; ++index) {
+ qpl_job* qpl_job_ptr =
+ reinterpret_cast<qpl_job*>(hw_jobs_buffer.get() + index * job_size);
+ if (qpl_init_job(qpl_path_hardware, qpl_job_ptr) != QPL_STS_OK) {
+ iaa_job_ready = false;
+ return arrow::Status::Invalid(
+ "Initialization of hardware IAA failed."
+ " Please check if Intel In-Memory Analytics Accelerator (IAA) "
+ "is properly set up!");
+ }
+ hw_job_ptr_pool[index] = qpl_job_ptr;
+ job_ptr_locks[index].store(false);
+ }
+
+ iaa_job_ready = true;
+ return arrow::Status::OK();
+}
+
+qpl_job* QplJobHWPool::AcquireJob(uint32_t& job_id) {
+ if (!job_ready()) {
+ return nullptr;
+ }
+ uint32_t retry = 0;
+ auto index = distribution(random_engine);
Review Comment:
Why do we use `distribution()` to find a free job?
Is it efficient?
##########
cpp/src/arrow/util/qpl_job_pool.h:
##########
@@ -0,0 +1,79 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <random>
+#include <utility>
+#include <vector>
+#include "arrow/config.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+#include "qpl/qpl.h"
+#include "qpl/qpl.hpp"
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+/// QplJobHWPool is resource pool to provide the job objects, which is
+/// used for storing context information during.
+/// Memory for QPL job will be allocated when the QPLJobHWPool instance is created
+///
+// QPL job can offload RLE-decoding/Filter/(De)compression works to hardware accelerator.
+class QplJobHWPool {
+ public:
+ static QplJobHWPool& GetInstance();
+
+ /// Acquire QPL job
+ ///
+ /// @param job_id QPL job id, used when release QPL job
+ /// \return Pointer to the QPL job. If acquire job failed, return nullptr.
+ qpl_job* AcquireJob(uint32_t& job_id);
+
+ /// \brief Release QPL job by the job_id.
+ void ReleaseJob(uint32_t job_id);
+
+ /// \brief Return if the QPL job is allocated sucessfully.
+ const bool& job_ready() { return iaa_job_ready; }
Review Comment:
Do we need `&` here?
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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/util/qpl_job_pool.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::GetInstance() {
+ static QplJobHWPool pool;
+ return pool;
+}
+
+QplJobHWPool::QplJobHWPool()
+ : random_engine(std::random_device()()), distribution(0, MAX_JOB_NUMBER - 1) {
+ (void)AllocateQPLJob();
+}
+
+QplJobHWPool::~QplJobHWPool() {
+ for (uint32_t i = 0; i < MAX_JOB_NUMBER; ++i) {
+ if (hw_job_ptr_pool[i]) {
+ qpl_fini_job(hw_job_ptr_pool[i]);
+ hw_job_ptr_pool[i] = nullptr;
+ }
+ }
+ iaa_job_ready = false;
+}
+
+arrow::Status QplJobHWPool::AllocateQPLJob() {
+ uint32_t job_size = 0;
+
+ /// Get size required for saving a single qpl job object
+ qpl_get_job_size(qpl_path_hardware, &job_size);
+ /// Allocate entire buffer for storing all job objects
+ hw_jobs_buffer = std::make_unique<uint8_t[]>(job_size * MAX_JOB_NUMBER);
+ /// Initialize pool for storing all job object pointers
+ /// Reallocate buffer by shifting address offset for each job object.
+ for (uint32_t index = 0; index < MAX_JOB_NUMBER; ++index) {
+ qpl_job* qpl_job_ptr =
+ reinterpret_cast<qpl_job*>(hw_jobs_buffer.get() + index * job_size);
+ if (qpl_init_job(qpl_path_hardware, qpl_job_ptr) != QPL_STS_OK) {
+ iaa_job_ready = false;
+ return arrow::Status::Invalid(
+ "Initialization of hardware IAA failed."
+ " Please check if Intel In-Memory Analytics Accelerator (IAA) "
+ "is properly set up!");
+ }
+ hw_job_ptr_pool[index] = qpl_job_ptr;
+ job_ptr_locks[index].store(false);
+ }
+
+ iaa_job_ready = true;
+ return arrow::Status::OK();
+}
+
+qpl_job* QplJobHWPool::AcquireJob(uint32_t& job_id) {
+ if (!job_ready()) {
+ return nullptr;
+ }
+ uint32_t retry = 0;
+ auto index = distribution(random_engine);
+ while (!tryLockJob(index)) {
+ index = distribution(random_engine);
+ retry++;
+ if (retry > MAX_JOB_NUMBER) {
+ return nullptr;
+ }
+ }
+ job_id = MAX_JOB_NUMBER - index;
Review Comment:
Why do we need to use different value for `index` and `job_id`?
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
Review Comment:
Sorry. I couldn't understand.
If we return `-1` here, decoding Parquet data is succeeded? (I thought decoding Parquet data is failed.)
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
Why do we need `- 1` here?
Does it touch invalid memory?
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
+ job->available_in = max_bytes_ + 1;
+
+ qpl_status status = qpl_execute_job(job);
Review Comment:
```suggestion
auto status = qpl_execute_job(job);
```
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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/util/qpl_job_pool.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::GetInstance() {
+ static QplJobHWPool pool;
+ return pool;
+}
+
+QplJobHWPool::QplJobHWPool()
+ : random_engine(std::random_device()()), distribution(0, MAX_JOB_NUMBER - 1) {
+ (void)AllocateQPLJob();
+}
+
+QplJobHWPool::~QplJobHWPool() {
+ for (uint32_t i = 0; i < MAX_JOB_NUMBER; ++i) {
+ if (hw_job_ptr_pool[i]) {
+ qpl_fini_job(hw_job_ptr_pool[i]);
+ hw_job_ptr_pool[i] = nullptr;
+ }
+ }
+ iaa_job_ready = false;
+}
+
+arrow::Status QplJobHWPool::AllocateQPLJob() {
+ uint32_t job_size = 0;
+
+ /// Get size required for saving a single qpl job object
+ qpl_get_job_size(qpl_path_hardware, &job_size);
+ /// Allocate entire buffer for storing all job objects
+ hw_jobs_buffer = std::make_unique<uint8_t[]>(job_size * MAX_JOB_NUMBER);
+ /// Initialize pool for storing all job object pointers
+ /// Reallocate buffer by shifting address offset for each job object.
+ for (uint32_t index = 0; index < MAX_JOB_NUMBER; ++index) {
+ qpl_job* qpl_job_ptr =
+ reinterpret_cast<qpl_job*>(hw_jobs_buffer.get() + index * job_size);
+ if (qpl_init_job(qpl_path_hardware, qpl_job_ptr) != QPL_STS_OK) {
+ iaa_job_ready = false;
+ return arrow::Status::Invalid(
+ "Initialization of hardware IAA failed."
+ " Please check if Intel In-Memory Analytics Accelerator (IAA) "
+ "is properly set up!");
+ }
+ hw_job_ptr_pool[index] = qpl_job_ptr;
+ job_ptr_locks[index].store(false);
+ }
+
+ iaa_job_ready = true;
+ return arrow::Status::OK();
+}
+
+qpl_job* QplJobHWPool::AcquireJob(uint32_t& job_id) {
+ if (!job_ready()) {
+ return nullptr;
+ }
+ uint32_t retry = 0;
+ auto index = distribution(random_engine);
+ while (!tryLockJob(index)) {
+ index = distribution(random_engine);
+ retry++;
+ if (retry > MAX_JOB_NUMBER) {
+ return nullptr;
+ }
+ }
+ job_id = MAX_JOB_NUMBER - index;
+ if (index >= MAX_JOB_NUMBER) {
+ return nullptr;
+ }
+ return hw_job_ptr_pool[index];
+}
+
+void QplJobHWPool::ReleaseJob(uint32_t job_id) {
+ if (job_ready()) {
+ job_ptr_locks[MAX_JOB_NUMBER - job_id].store(false);
+ }
+}
+
+bool QplJobHWPool::tryLockJob(uint32_t index) {
Review Comment:
```suggestion
bool QplJobHWPool::TryLockJob(uint32_t index) {
```
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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/util/qpl_job_pool.h"
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::GetInstance() {
+ static QplJobHWPool pool;
+ return pool;
+}
+
+QplJobHWPool::QplJobHWPool()
+ : random_engine(std::random_device()()), distribution(0, MAX_JOB_NUMBER - 1) {
+ (void)AllocateQPLJob();
+}
+
+QplJobHWPool::~QplJobHWPool() {
+ for (uint32_t i = 0; i < MAX_JOB_NUMBER; ++i) {
+ if (hw_job_ptr_pool[i]) {
+ qpl_fini_job(hw_job_ptr_pool[i]);
+ hw_job_ptr_pool[i] = nullptr;
+ }
+ }
+ iaa_job_ready = false;
+}
+
+arrow::Status QplJobHWPool::AllocateQPLJob() {
+ uint32_t job_size = 0;
+
+ /// Get size required for saving a single qpl job object
+ qpl_get_job_size(qpl_path_hardware, &job_size);
+ /// Allocate entire buffer for storing all job objects
+ hw_jobs_buffer = std::make_unique<uint8_t[]>(job_size * MAX_JOB_NUMBER);
+ /// Initialize pool for storing all job object pointers
+ /// Reallocate buffer by shifting address offset for each job object.
+ for (uint32_t index = 0; index < MAX_JOB_NUMBER; ++index) {
+ qpl_job* qpl_job_ptr =
Review Comment:
```suggestion
auto qpl_job_ptr =
```
##########
cpp/thirdparty/versions.txt:
##########
@@ -76,6 +76,8 @@ ARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=2f723218f6cb709ae4cdc4fb5ed56a5951fc5d466f0
# warnings.
ARROW_RAPIDJSON_BUILD_VERSION=232389d4f1012dddec4ef84861face2d2ba85709
ARROW_RAPIDJSON_BUILD_SHA256_CHECKSUM=b9290a9a6d444c8e049bd589ab804e0ccf2b05dc5984a19ed5ae75d090064806
+ARROW_QPL_BUILD_VERSION=v0.2.1
Review Comment:
It seem that 0.3.0 is the latest release: https://github.com/intel/qpl/releases/tag/v0.3.0
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
Review Comment:
Why do we need to introduce new `value_offset_`? Can we use existing `bit_offset_` and `byte_offset_` instead?
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,73 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, const std::vector<uint8_t>& destination,
+ T* values, int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<const V*>(destination.data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ auto* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (!job) {
+ return -1;
+ }
+
+ std::vector<uint8_t> destination;
+ if (dictionary_length < 0xFF) {
+ job->out_bit_width = qpl_ow_8;
+ destination.resize(batch_size, 0);
+ } else if (dictionary_length < 0xFFFF) {
+ job->out_bit_width = qpl_ow_16;
+ destination.resize(batch_size * 2, 0);
+ } else {
+ job->out_bit_width = qpl_ow_32;
+ destination.resize(batch_size * 4, 0);
+ }
+
+ job->op = qpl_op_extract;
+ job->src1_bit_width = bit_width_;
+ job->parser = qpl_p_parquet_rle;
+ job->next_out_ptr = destination.data();
+ job->available_out = static_cast<uint32_t>(destination.size());
+
+ if (!bit_reader_.GetBatchWithQpl(batch_size, job)) {
+ return -1;
Review Comment:
Should we call `qpl_fini_job()` and `ReleaseJob()` here too?
Can we use RAII to ensure releasing job like `std::lock()`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,51 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND)
+ find_package(Patch)
+ if(Patch_FOUND)
+ # This patch is for Qpl <= v0.2.0
+ set(QPL_PATCH_COMMAND
+ ${Patch_EXECUTABLE}
+ "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep-prefix/src/qpl_ep/tools/CMakeLists.txt"
+ "${CMAKE_SOURCE_DIR}/build-support/qpl-tools-cmakefile.patch")
+ endif()
+
+ externalproject_add(qpl_ep
+ ${EP_LOG_OPTIONS}
+ URL ${QPL_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_QPL_BUILD_SHA256_CHECKSUM}"
+ PATCH_COMMAND ${QPL_PATCH_COMMAND}
+ BUILD_BYPRODUCTS "${QPL_STATIC_LIB}"
+ CMAKE_ARGS ${QPL_CMAKE_ARGS})
+
+ file(MAKE_DIRECTORY "${QPL_PREFIX}/include")
+
+ add_library(Qpl::qpl STATIC IMPORTED)
+ set(QPL_LIBRARIES ${QPL_STATIC_LIB})
+ set(QPL_INCLUDE_DIRS "${QPL_PREFIX}/include")
+ set_target_properties(Qpl::qpl
+ PROPERTIES IMPORTED_LOCATION ${QPL_LIBRARIES}
+ INTERFACE_INCLUDE_DIRECTORIES ${QPL_INCLUDE_DIRS})
Review Comment:
We can remove needless variables here:
```suggestion
set_target_properties(Qpl::qpl
PROPERTIES IMPORTED_LOCATION ${QPL_STATIC_LIB}
INTERFACE_INCLUDE_DIRECTORIES ${QPL_PREFIX}/include)
```
##########
cpp/src/parquet/encoding.cc:
##########
@@ -1494,8 +1494,13 @@ class DictDecoderImpl : public DecoderImpl, virtual public DictDecoder<Type> {
int Decode(T* buffer, int num_values) override {
num_values = std::min(num_values, num_values_);
int decoded_values =
+#ifdef ARROW_WITH_QPL
+ idx_decoder_.GetBatchWithDictIAA(reinterpret_cast<const T*>(dictionary_->data()),
+ dictionary_length_, buffer, num_values);
Review Comment:
Can we dispatch in `GetBatchWithDict()` not here?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,51 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND)
+ find_package(Patch)
+ if(Patch_FOUND)
+ # This patch is for Qpl <= v0.2.0
Review Comment:
Did you upstream this patch?
Please add pull request URL as comment.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1030859062
##########
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:
We want to use a CMake target instead of `add_definitions()` like other third party dependency.
##########
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(ENABLE_QPL "Enable Intel® Query Processing Library" OFF)
Review Comment:
We use `ARROW_${FEATURE}` for option name.
##########
cpp/CMakeLists.txt:
##########
@@ -528,6 +534,12 @@ message(STATUS "CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}")
include_directories(${CMAKE_CURRENT_BINARY_DIR}/src)
include_directories(src)
+if(ENABLE_QPL)
+ set (QPL_PROJECT_DIR "${PROJECT_SOURCE_DIR}/submodules/qpl")
+ include_directories(${QPL_PROJECT_DIR})
Review Comment:
We want to use a CMake target instead of `include_directories()` like other third party dependency.
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,118 @@
+// 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 <exception>
Review Comment:
We use `arrow::Status` and `arrow::Result` instead of C++ exception to report an error.
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,118 @@
+// 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 <exception>
+#include "parquet/exception.h"
Review Comment:
We don't want to depend on `cpp/src/parquet/` in `cpp/src/arrow/util/`.
##########
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)
Review Comment:
We want to use `externalproject_add()` in `cpp/cmake_modules/ThirdpartyToolchain.cmake` instead of `add_subdirectory()` like other third party dependency.
##########
.gitmodules:
##########
@@ -4,3 +4,6 @@
[submodule "testing"]
path = testing
url = https://github.com/apache/arrow-testing
+[submodule "cpp/submodules/qpl"]
+ path = cpp/submodules/qpl
+ url = https://github.com/intel/qpl.git
Review Comment:
We don't want to use submodule for a third party dependency.
Could you use `externalproject_add()` like other third party dependencies?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1039075100
##########
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:
> When the next release is scheduled?
It would be fixed in Q1, 2023
> BTW, we can apply a patch with externalproject_add():
Thanks for your guide. I have add a patch with externalproject_add
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045347665
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
Review Comment:
There is no public path on https://github.com/intel/qpl.
The problem of this release package is the lack of google-test submodules. Is it ok to use _wget_ the google-test package in the patch command?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
+ "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep-prefix/src/qpl_ep/tools/CMakeLists.txt")
+
+ externalproject_add(qpl_ep
+ ${EP_LOG_OPTIONS}
+ URL ${QPL_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_QPL_BUILD_SHA256_CHECKSUM}"
+ PATCH_COMMAND ${QPL_PATCH_COMMAND}
+ BUILD_BYPRODUCTS "${QPL_STATIC_LIB}"
+ CMAKE_ARGS ${QPL_CMAKE_ARGS})
+
+ file(MAKE_DIRECTORY "${QPL_PREFIX}/include")
+
+ add_library(qpl::qpl STATIC IMPORTED)
Review Comment:
sure, done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045354335
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
+ }
+
+ std::vector<uint8_t>* destination;
+ if (dictionary_length < 0xFF) {
+ job->out_bit_width = qpl_ow_8;
+ destination = new std::vector<uint8_t>(batch_size, 0);
Review Comment:
your suggestion is useful! Update complete.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045349408
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
Review Comment:
I was thinking that if the QPL Job was not ready, it would not affect the RLE-Decoding
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045349408
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
Review Comment:
I was thinking that if the QPL Job was not ready, it would not affect the final RLE-Decoding
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1328570981
@kou Hi, kou.
1) I have updated the code that you commented last week.
2) As for the maintenance of this feature, I add a runtime check the IAA device. If CPU processor of the machine support IAA, the continuous-integration will check the code. After the launching of the Intel CPU processor, the CI is able to check this feature.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
emkornfield commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1316699705
@yaqi-zhao I'm sorry for the confusion it isn't about me having access to a test machine. The issue is being able to continuosly test this code with github actions to make sure there is no regressions. I'm not sure if github actions supports having custom machines integrated with it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1334324641
It seems there are still CMake lint errors: https://github.com/apache/arrow/actions/runs/3591159963/jobs/6045602036#step:5:815
```text
INFO:archery:Running cmake-format linters
ERROR __main__.py:618: Check failed: /arrow/cpp/cmake_modules/DefineOptions.cmake
ERROR __main__.py:618: Check failed: /arrow/cpp/CMakeLists.txt
ERROR __main__.py:618: Check failed: /arrow/cpp/cmake_modules/FindQPL.cmake
ERROR __main__.py:618: Check failed: /arrow/cpp/cmake_modules/ThirdpartyToolchain.cmake
```
Could you confirm them?
https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci
> CMake files pass style checks, can be fixed by running `archery lint --cmake-format --fix`. This requires Python 3 and [cmake_format](https://github.com/cheshirekow/cmake_format) (note: this currently does not work on Windows).
FYI: You can check lint result in your fork too: https://github.com/yaqi-zhao/arrow/actions/runs/3591159610/jobs/6045360960#step:5:4197
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
emkornfield commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1304394226
Two high level concerns:
1. Apologies if I missed it but this doesn't seem to integrate with CI so it will likely not super maintainable.
2. Adding a new library dependency might not be desirable. In particular it looks like this library is still in beta?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045376900
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -190,6 +195,14 @@ class BitReader {
/// Maximum byte length of a vlq encoded int64
static constexpr int kMaxVlqByteLengthForInt64 = 10;
+ const uint8_t* GetBuffer() { return buffer_ - 1; }
+
+ int GetBufferLen() { return max_bytes_ + 1; }
+
+ void updateValOffset(int batch_size) { val_offset_ += batch_size; }
+
+ int GetValOffset() { return val_offset_; }
Review Comment:
It makes sense. I have add GetBatchWithQpl in bit_stream_utils.h and not expose the variables in BitReader. Please take a look.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045377150
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -633,6 +636,14 @@ else()
"${THIRDPARTY_MIRROR_URL}/protobuf-${ARROW_PROTOBUF_BUILD_VERSION}.tar.gz")
endif()
+if(DEFINED ENV{ARROW_QPL_URL})
+ set(QPL_SOURCE_URL "$ENV{ARROW_QPL_URL}")
+else()
+ set_urls(QPL_SOURCE_URL
+ "https://github.com/intel/qpl/archive/refs/tags/${ARROW_QPL_BUILD_VERSION}.tar.gz"
+ "${THIRDPARTY_MIRROR_URL}/qpl-${ARROW_QPL_BUILD_VERSION}.tar.gz")
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1039075100
##########
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:
> When the next release is scheduled?
It would be fixed in Q1, 2023
> BTW, we can apply a patch with externalproject_add():
I have add a patch with externalproject_add
##########
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:
> When the next release is scheduled?
It would be fixed in Q1, 2023
> BTW, we can apply a patch with externalproject_add():
I have add a patch with externalproject_add
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1039072175
##########
cpp/src/arrow/util/qpl_job_pool.cc:
##########
@@ -0,0 +1,122 @@
+// 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 ARROW_WITH_QPL
+#include "arrow/util/qpl_job_pool.h"
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+std::array<qpl_job*, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::hw_job_ptr_pool;
+std::array<std::atomic_bool, QplJobHWPool::MAX_JOB_NUMBER> QplJobHWPool::job_ptr_locks;
+bool QplJobHWPool::iaa_job_ready = false;
+std::unique_ptr<uint8_t[]> QplJobHWPool::hw_jobs_buffer;
+
+QplJobHWPool& QplJobHWPool::instance() {
+ static QplJobHWPool pool;
Review Comment:
If the query concurrency of the client is high, initialization of the qpl job will take a long time. This pool tend to save time on job initialization.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034438074
##########
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:
no need for this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1330308094
> Does this mean that we can build this code without the new Intel CPU?
@kou Yes, QPL has no requirement for CPU, it can be built without the new Intel CPU.
QPL can check if IAA is enabled on the CPU which the program is running on.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1316519433
@emkornfield hi, emkornfield. For the 2 questions you proposed last week, is the solution on comment acceptable?
> https://github.com/apache/arrow/pull/14585#issuecomment-1305276128
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] maqister commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
maqister commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1328606308
> @maqister We run benchmark test on ReadRowGroups APIs, test results show that there are 1~2X perf improvements according to different rle-encoded bit width of parquet file.
> arrow with iaa<img alt="image" width="1224" src="https://user-images.githubusercontent.com/100556669/203264126-972646a7-59d6-4911-83da-2adc8fdb4de6.png">
> arrow master <img alt="image" width="1215" src="https://user-images.githubusercontent.com/100556669/203264181-974c4162-03f4-4a64-a582-f4568a6ef084.png">
thanks a lot for sharing the results!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1039075100
##########
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:
> When the next release is scheduled?
It would be fixed in Q1, 2023
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045629841
##########
cpp/CMakeLists.txt:
##########
@@ -796,6 +796,15 @@ if(ARROW_USE_XSIMD)
list(APPEND ARROW_STATIC_LINK_LIBS xsimd)
endif()
+if(ARROW_WITH_QPL)
+ add_definitions(-DARROW_WITH_QPL)
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048116742
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
Oh, does it mean that we can use this feature only for Parquet's RLE format?
Could you tell me IAA's document for this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048177840
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
> Oh, does it mean that we can use this feature only for Parquet's RLE format? Could you tell me IAA's document for this?
Hi, Kou. You can download IAA specification from https://www.intel.com/content/www/us/en/content-details/721858/intel-in-memory-analytics-accelerator-architecture-specification.html?wapkw=In-memory%20accelerator
And this PR is only for Parquet's RLE format.
> Or is "bit-width" included in general RLE format?
Yes, it's a general format, you can read this format from https://parquet.apache.org/docs/file-format/data-pages/encodings/#a-namerlearun-length-encoding--bit-packing-hybrid-rle--3
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048109312
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
It will not touch invlalid memory since in function [DictDecoderImpl::SetData](https://github.com/apache/arrow/blob/f6685371032e933e31d7ae2b156a6385c00d1673/cpp/src/parquet/encoding.cc#L1491), bit reader buffer was set to<encoded-data> and skip <bit_width>
IAA need a full parquet rle buffer so I `-1` here
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
It will not touch invlalid memory since in function [DictDecoderImpl::SetData](https://github.com/apache/arrow/blob/f6685371032e933e31d7ae2b156a6385c00d1673/cpp/src/parquet/encoding.cc#L1491), bit reader buffer was set to <encoded-data> and skip <bit_width>
IAA need a full parquet rle buffer so I `-1` here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045348528
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -200,6 +213,7 @@ class BitReader {
int byte_offset_; // Offset in buffer_
int bit_offset_; // Offset in buffered_values_
+ int val_offset_; // Index of next value to read
Review Comment:
one
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1044813741
##########
cpp/CMakeLists.txt:
##########
@@ -796,6 +796,15 @@ if(ARROW_USE_XSIMD)
list(APPEND ARROW_STATIC_LINK_LIBS xsimd)
endif()
+if(ARROW_WITH_QPL)
+ add_definitions(-DARROW_WITH_QPL)
Review Comment:
Could you put this to `cpp/src/arrow/util/config.h.cmake` instead of here?
##########
cpp/cmake_modules/FindQPL.cmake:
##########
@@ -0,0 +1,60 @@
+# 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.
+
+# - Find Intel®QPL
+# This module defines
+# QPL_ROOT - When set, this path is inspected instead of standard library
+# QPL_INCLUDE_DIR, directory containing headers
+# QPL_STATIC_LIB, path to libqpl.a
+# QPL_FOUND, whether QPL is found or not
Review Comment:
We should mark only CMake target (`qpl::qpl`) public API. Others should not mark public API.
##########
cpp/cmake_modules/FindQPL.cmake:
##########
@@ -0,0 +1,60 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Do we need this file?
It seems that QPL provides its CMake package: https://github.com/intel/qpl/blob/develop/CMakeLists.txt#L66-L68
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -64,6 +64,7 @@ set(ARROW_THIRDPARTY_DEPENDENCIES
ORC
re2
Protobuf
+ QPL
Review Comment:
It seems that QPL uses `Qpl` for its CMake package name: https://github.com/intel/qpl/blob/develop/CMakeLists.txt#L66-L68
```suggestion
Qpl
```
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -200,6 +213,7 @@ class BitReader {
int byte_offset_; // Offset in buffer_
int bit_offset_; // Offset in buffered_values_
+ int val_offset_; // Index of next value to read
Review Comment:
Please don't abbreviate `value` as `val` for readability and consistency?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
+ "${CMAKE_CURRENT_BINARY_DIR}/qpl_ep-prefix/src/qpl_ep/tools/CMakeLists.txt")
+
+ externalproject_add(qpl_ep
+ ${EP_LOG_OPTIONS}
+ URL ${QPL_SOURCE_URL}
+ URL_HASH "SHA256=${ARROW_QPL_BUILD_SHA256_CHECKSUM}"
+ PATCH_COMMAND ${QPL_PATCH_COMMAND}
+ BUILD_BYPRODUCTS "${QPL_STATIC_LIB}"
+ CMAKE_ARGS ${QPL_CMAKE_ARGS})
+
+ file(MAKE_DIRECTORY "${QPL_PREFIX}/include")
+
+ add_library(qpl::qpl STATIC IMPORTED)
Review Comment:
Could you use `Qpl::qpl` because QPL's CMake package uses `Qpl::qpl`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
Review Comment:
`sed -i` (no suffix) isn't portable. It works with GNU sed but not with BSD sed.
BTW, this approach is too fragile. How about using `patch` command like a code in `build_google_cloud_cpp_storage()`?
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -633,6 +636,14 @@ else()
"${THIRDPARTY_MIRROR_URL}/protobuf-${ARROW_PROTOBUF_BUILD_VERSION}.tar.gz")
endif()
+if(DEFINED ENV{ARROW_QPL_URL})
+ set(QPL_SOURCE_URL "$ENV{ARROW_QPL_URL}")
+else()
+ set_urls(QPL_SOURCE_URL
+ "https://github.com/intel/qpl/archive/refs/tags/${ARROW_QPL_BUILD_VERSION}.tar.gz"
+ "${THIRDPARTY_MIRROR_URL}/qpl-${ARROW_QPL_BUILD_VERSION}.tar.gz")
Review Comment:
We don't need a mirror URL here because we don't prepare it.
```suggestion
"https://github.com/intel/qpl/archive/refs/tags/${ARROW_QPL_BUILD_VERSION}.tar.gz")
```
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
+ }
+
+ std::vector<uint8_t>* destination;
+ if (dictionary_length < 0xFF) {
+ job->out_bit_width = qpl_ow_8;
+ destination = new std::vector<uint8_t>(batch_size, 0);
Review Comment:
Who does free this? QPL?
Can we use `std::vector<uint8_t> destination` and `std::vector::resize()` instead of `new std::vector<uint8_t>`?
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
+ return -1;
Review Comment:
Should we fall back to `GetBatchWithDict()`?
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
Review Comment:
```suggestion
auto job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
```
##########
cpp/src/parquet/CMakeLists.txt:
##########
@@ -127,7 +127,6 @@ set(PARQUET_SHARED_TEST_LINK_LIBS arrow_testing_shared ${PARQUET_MIN_TEST_LIBS}
set(PARQUET_STATIC_TEST_LINK_LIBS ${PARQUET_MIN_TEST_LIBS} parquet_static thrift::thrift
${ARROW_LIBRARIES_FOR_STATIC_TESTS})
-
Review Comment:
Could you revert this to reduce needless diff?
##########
cpp/thirdparty/versions.txt:
##########
@@ -96,7 +98,6 @@ ARROW_ZLIB_BUILD_SHA256_CHECKSUM=b3a24de97a8fdbc835b9833169501030b8977031bcb54b3
ARROW_ZSTD_BUILD_VERSION=v1.5.2
ARROW_ZSTD_BUILD_SHA256_CHECKSUM=f7de13462f7a82c29ab865820149e778cbfe01087b3a55b5332707abf9db4a6e
-
Review Comment:
Could you revert this to reduce needless diff?
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -190,6 +195,14 @@ class BitReader {
/// Maximum byte length of a vlq encoded int64
static constexpr int kMaxVlqByteLengthForInt64 = 10;
+ const uint8_t* GetBuffer() { return buffer_ - 1; }
+
+ int GetBufferLen() { return max_bytes_ + 1; }
+
+ void updateValOffset(int batch_size) { val_offset_ += batch_size; }
+
+ int GetValOffset() { return val_offset_; }
Review Comment:
I don't think that exporting internal buffer and changing offset from outside is a good API.
This is a reader class. So this is responsible to manage them. This API just uses this as a data container.
Can we choose another approach? For example, can we use QPL in this?
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
Review Comment:
```suggestion
if (!job) {
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045347665
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
Review Comment:
There is no public path on https://github.com/intel/qpl.
The problem of this release package is the lack of google-test submodules. Is it ok to use _wget_ the google-test package in the patch command?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048109312
##########
cpp/src/arrow/util/bit_stream_utils.h:
##########
@@ -398,6 +412,27 @@ inline int BitReader::GetBatch(int num_bits, T* v, int batch_size) {
return batch_size;
}
+#ifdef ARROW_WITH_QPL
+inline bool BitReader::GetBatchWithQpl(int batch_size, qpl_job* job) {
+ if (!job) {
+ return false;
+ }
+ job->param_low = value_offset_;
+ job->param_high = batch_size + value_offset_;
+ job->num_input_elements = batch_size + value_offset_;
+
+ job->next_in_ptr = const_cast<uint8_t*>(buffer_ - 1);
Review Comment:
It will not touch invlalid memory since in function [DictDecoderImpl::SetData](https://github.com/apache/arrow/blob/f6685371032e933e31d7ae2b156a6385c00d1673/cpp/src/parquet/encoding.cc#L1491), bit reader buffer was set to <encoded-data> and skip <bit_width>
IAA need a full parquet rle buffer so I `-1` here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048123988
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,51 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND)
+ find_package(Patch)
+ if(Patch_FOUND)
+ # This patch is for Qpl <= v0.2.0
Review Comment:
We can use a patch that is not yet merged but we don't want to maintain patches to reduce maintenance cost.
So we must upstream our patches.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1305276128
Thanks for your comments, it helps a lot. @emkornfield
For Q1： I'll try to add testing to CI, but this feature requires IAA(Intel® In-Memory Analytics Accelerator) hardware, which is a new built-in accelerator in the next Gen Intel® Xeon® Scalable processors. The next Gen Intel® Xeon® Scalable processors will lunch in the near future. Maybe we run the CI together in Intel's lab machine environment that supports this processor.
For Q2: QPL is a necessary library to enable IAA and the library will provide a release version with the lunch of the next Gen Intel® Xeon® Scalable processors.
If a new library dependency might not be desirable, do you think the following solution will be better?
1. installing the library directly on the machine instead of adding an explicit dependency in Arrow
2. Add a third-party toolchain just like the implementation of Snappy, ZSTD, etc.
3. I realize the used API in QPL library instead of adding a new library dependency, but this will be many new codes to be merged.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1321666402
@maqister We run benchmark test on ReadRowGroups APIs, test results show that there are 1~3X perf improvements according to different rle-encoded bit width of parquet file.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1030966775
##########
.gitmodules:
##########
@@ -4,3 +4,6 @@
[submodule "testing"]
path = testing
url = https://github.com/apache/arrow-testing
+[submodule "cpp/submodules/qpl"]
+ path = cpp/submodules/qpl
+ url = https://github.com/intel/qpl.git
Review Comment:
ok, i'll change to externalproject_add() to add this dependencies
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1336785406
Hi, @kou and @wgtmac . I have run success the lint check on my fork, could you please help continue the review?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wgtmac commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
wgtmac commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1041884354
##########
cpp/src/arrow/util/qpl_job_pool.h:
##########
@@ -0,0 +1,68 @@
+// 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.
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <random>
+#include <utility>
+#include <vector>
+#include "arrow/status.h"
+
+#ifdef ARROW_WITH_QPL
+#include "qpl/qpl.h"
+#include "qpl/qpl.hpp"
+
+namespace arrow {
+namespace util {
+namespace internal {
+
+/// QplJobHWPool is resource pool to provide the job objects.
+/// Job object is used for storing context information during
+// offloading compression job to HW Accelerator.
+class QplJobHWPool {
+ public:
+ QplJobHWPool();
+ ~QplJobHWPool();
+
+ static QplJobHWPool& instance();
Review Comment:
Function names here look more like the Java style. I'd suggest to follow the convention like `arrow/memory_pool.h` and add more comments about the public APIs. BTW, is it possible to refactor the static functions to be member functions? You may simply use a singleton to call member functions. cc @pitrou
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045629551
##########
cpp/cmake_modules/FindQPL.cmake:
##########
@@ -0,0 +1,60 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
You're right. I tried using intel/qpl installed package on Linux and find_package succeeded.
FindQPL.cmake is no need.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034422002
##########
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:
no need for this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1034426336
##########
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:
There is a known issue on intel/qpl:
QPL cannot be built from direct downloadable files (.tar, .tgz) since it has submodules which are not included to the archives by GitHub during release creation(https://github.com/intel/qpl/releases). This will be solved on next version.
For now, can we just use git repository to pull released code? Maybe we can create an issue/Jira to track this problem if the code is merged.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1333460457
Hi, @kou. I have fixed the lint error of this patch, could you please help review it again?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1316576484
@emkornfield I'll find a machine environment on Intel's lab that you can access as soon as possible and once the machine is ready I'll share the machine on email to you.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] martin-g commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1356884811
About the missing CI: How about using a self-hosted runner donated by Intel ?
@assignUser already started a discussion about using ephemeral self-hosted runners at https://lists.apache.org/thread/mskpqwpdq65t1wpj4f5klfq9217ljodw
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1350460281
@kou Thanks for your comments and the code have been updated. Please take a look, thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1048119591
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,51 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND)
+ find_package(Patch)
+ if(Patch_FOUND)
+ # This patch is for Qpl <= v0.2.0
Review Comment:
This was not unstreamed. Do we have to use patches that have already been merged?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045348697
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
Review Comment:
done
##########
cpp/src/arrow/util/rle_encoding.h:
##########
@@ -598,6 +605,83 @@ inline int RleDecoder::GetBatchWithDict(const T* dictionary, int32_t dictionary_
return values_read;
}
+#ifdef ARROW_WITH_QPL
+template <typename T, typename V>
+inline void CopyValues(const T* dictionary, std::vector<uint8_t>* destination, T* values,
+ int batch_size) {
+ auto* out = values;
+ auto* indices = reinterpret_cast<V*>(destination->data());
+ for (int j = 0; j < batch_size; j++) {
+ auto idx = indices[j];
+ T val = dictionary[idx];
+ std::fill(out, out + 1, val);
+ out++;
+ }
+ return;
+}
+
+template <typename T>
+inline int RleDecoder::GetBatchWithDictIAA(const T* dictionary, int32_t dictionary_length,
+ T* values, int batch_size) {
+ if (batch_size <= 0) {
+ return batch_size;
+ }
+ if (!::arrow::util::internal::QplJobHWPool::GetInstance().job_ready() ||
+ bit_width_ <= 1) {
+ return GetBatchWithDict(dictionary, dictionary_length, values, batch_size);
+ }
+ uint32_t job_id = 0;
+ qpl_job* job = ::arrow::util::internal::QplJobHWPool::GetInstance().AcquireJob(job_id);
+ if (job == NULL) {
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045354619
##########
cpp/thirdparty/versions.txt:
##########
@@ -96,7 +98,6 @@ ARROW_ZLIB_BUILD_SHA256_CHECKSUM=b3a24de97a8fdbc835b9833169501030b8977031bcb54b3
ARROW_ZSTD_BUILD_VERSION=v1.5.2
ARROW_ZSTD_BUILD_SHA256_CHECKSUM=f7de13462f7a82c29ab865820149e778cbfe01087b3a55b5332707abf9db4a6e
-
Review Comment:
done
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] yaqi-zhao commented on a diff in pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by GitBox <gi...@apache.org>.
yaqi-zhao commented on code in PR #14585:
URL: https://github.com/apache/arrow/pull/14585#discussion_r1045488871
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2203,6 +2214,45 @@ 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")
+ set(QPL_STATIC_LIB_NAME ${CMAKE_STATIC_LIBRARY_PREFIX}qpl${CMAKE_STATIC_LIBRARY_SUFFIX})
+ set(QPL_STATIC_LIB "${QPL_PREFIX}/lib/${QPL_STATIC_LIB_NAME}")
+ set(QPL_CMAKE_ARGS ${EP_COMMON_CMAKE_ARGS} -DCMAKE_INSTALL_LIBDIR=lib
+ "-DCMAKE_INSTALL_PREFIX=${QPL_PREFIX}")
+ set(QPL_PATCH_COMMAND
+ "sed" "-i" "17,18d"
Review Comment:
done with adding qpl-tools-cmakefile.patch
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #14585: ARROW-17884: [C++] Add Intel®-IAA/QPL-based Parquet RLE Decode
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #14585:
URL: https://github.com/apache/arrow/pull/14585#issuecomment-1490645814
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org