Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
pitrou committed Jul 17, 2023
1 parent 6171db8 commit e954e65
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 127 deletions.
8 changes: 4 additions & 4 deletions .github/workflows/cpp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ env:

jobs:
docker:
name: ${{ matrix.title }} ${{ matrix.simd-level }}
name: ${{ matrix.title }}
runs-on: ${{ matrix.runs-on }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
timeout-minutes: 75
Expand All @@ -68,7 +68,7 @@ jobs:
llvm: "14"
runs-on: ubuntu-latest
simd-level: AVX2
title: AMD64 Conda C++
title: AMD64 Conda C++ AVX2
ubuntu: "22.04"
- arch: amd64
clang-tools: "14"
Expand Down Expand Up @@ -226,7 +226,7 @@ jobs:
ci/scripts/cpp_test.sh $(pwd) $(pwd)/build
windows:
name: AMD64 ${{ matrix.name }} C++17 ${{ matrix.simd-level }}
name: ${{ matrix.title }}
runs-on: ${{ matrix.os }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
timeout-minutes: 60
Expand All @@ -237,8 +237,8 @@ jobs:
- windows-2019
include:
- os: windows-2019
name: Windows 2019
simd-level: AVX2
title: AMD64 Windows 2019 C++17 AVX2
env:
ARROW_BOOST_USE_SHARED: OFF
ARROW_BUILD_BENCHMARKS: ON
Expand Down
94 changes: 39 additions & 55 deletions cpp/src/arrow/acero/bloom_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,18 @@
#include <condition_variable>
#include <thread>
#include <unordered_set>

#include "arrow/acero/bloom_filter.h"
#include "arrow/acero/task_util.h"
#include "arrow/acero/test_util_internal.h"
#include "arrow/acero/util.h"
#include "arrow/compute/key_hash.h"
#include "arrow/util/bitmap_ops.h"
#include "arrow/util/cpu_info.h"

namespace arrow {

using compute::Hashing32;
using compute::Hashing64;
using internal::CpuInfo;

namespace acero {

Expand Down Expand Up @@ -172,13 +171,7 @@ void TestBloomSmallHashHelper(int64_t num_input_hashes, const T* input_hashes,
// Output FPR and build and probe cost.
//
void TestBloomSmall(BloomFilterBuildStrategy strategy, int64_t num_build,
int num_build_copies, bool use_avx2, bool enable_prefetch) {
int64_t hardware_flags = use_avx2 ? CpuInfo::AVX2 : 0;
if (hardware_flags && !CpuInfo::GetInstance()->IsSupported(hardware_flags)) {
// What else?
return;
}

int num_build_copies, int64_t hardware_flags, bool enable_prefetch) {
// Generate input keys
//
int64_t num_probe = 4 * num_build;
Expand Down Expand Up @@ -329,14 +322,8 @@ void TestBloomLargeHashHelper(int64_t hardware_flags, int64_t block,
// Test with larger size Bloom filters (use large prime with arithmetic
// sequence modulo 2^64).
//
void TestBloomLarge(BloomFilterBuildStrategy strategy, int64_t num_build, bool use_avx2,
bool enable_prefetch) {
int64_t hardware_flags = use_avx2 ? CpuInfo::AVX2 : 0;
if (hardware_flags && !CpuInfo::GetInstance()->IsSupported(hardware_flags)) {
// What else?
return;
}

void TestBloomLarge(BloomFilterBuildStrategy strategy, int64_t num_build,
int64_t hardware_flags, bool enable_prefetch) {
// Largest 63-bit prime
constexpr uint64_t prime = 0x7FFFFFFFFFFFFFE7ULL;

Expand Down Expand Up @@ -467,42 +454,40 @@ TEST(BloomFilter, Basic) {
num_build.push_back(1LL << log_large);
#endif

constexpr int num_param_sets = 3;
struct {
bool use_avx2;
struct TestParam {
int64_t hardware_flags;
bool enable_prefetch;
bool insert_multiple_copies;
} params[num_param_sets];
for (int i = 0; i < num_param_sets; ++i) {
params[i].use_avx2 = (i == 1);
params[i].enable_prefetch = (i == 2);
params[i].insert_multiple_copies = (i == 3);
};
std::vector<TestParam> test_params;
for (const auto hardware_flags : HardwareFlagsForTesting()) {
test_params.push_back({hardware_flags, false, false});
}
test_params.push_back({0, true, false});
test_params.push_back({0, false, true});

std::vector<BloomFilterBuildStrategy> strategy;
strategy.push_back(BloomFilterBuildStrategy::SINGLE_THREADED);
std::vector<BloomFilterBuildStrategy> strategies;
strategies.push_back(BloomFilterBuildStrategy::SINGLE_THREADED);
#ifndef ARROW_VALGRIND
strategy.push_back(BloomFilterBuildStrategy::PARALLEL);
strategies.push_back(BloomFilterBuildStrategy::PARALLEL);
#endif

static constexpr int64_t min_rows_for_large = 2 * 1024 * 1024;

for (size_t istrategy = 0; istrategy < strategy.size(); ++istrategy) {
for (int iparam_set = 0; iparam_set < num_param_sets; ++iparam_set) {
ARROW_SCOPED_TRACE("%s ", params[iparam_set].use_avx2 ? "AVX2"
: params[iparam_set].enable_prefetch ? "PREFETCH"
: params[iparam_set].insert_multiple_copies ? "FOLDING"
: "REGULAR");
for (size_t inum_build = 0; inum_build < num_build.size(); ++inum_build) {
ARROW_SCOPED_TRACE("num_build ", static_cast<int>(num_build[inum_build]));
if (num_build[inum_build] >= min_rows_for_large) {
TestBloomLarge(strategy[istrategy], num_build[inum_build],
params[iparam_set].use_avx2, params[iparam_set].enable_prefetch);
for (const auto& strategy : strategies) {
for (const auto& test_param : test_params) {
ARROW_SCOPED_TRACE("hardware_flags = ", test_param.hardware_flags,
test_param.enable_prefetch ? " PREFETCH" : "",
test_param.insert_multiple_copies ? " FOLDING" : "REGULAR");
for (const auto n : num_build) {
ARROW_SCOPED_TRACE("num_build ", n);
if (n >= min_rows_for_large) {
TestBloomLarge(strategy, n, test_param.hardware_flags,
test_param.enable_prefetch);

} else {
TestBloomSmall(strategy[istrategy], num_build[inum_build],
params[iparam_set].insert_multiple_copies ? 8 : 1,
params[iparam_set].use_avx2, params[iparam_set].enable_prefetch);
TestBloomSmall(strategy, n, test_param.insert_multiple_copies ? 8 : 1,
test_param.hardware_flags, test_param.enable_prefetch);
}
}
}
Expand All @@ -515,19 +500,18 @@ TEST(BloomFilter, Scaling) {
num_build.push_back(1000000);
num_build.push_back(4000000);

std::vector<BloomFilterBuildStrategy> strategy;
strategy.push_back(BloomFilterBuildStrategy::PARALLEL);

for (bool use_avx2 : {false, true}) {
for (size_t istrategy = 0; istrategy < strategy.size(); ++istrategy) {
for (size_t inum_build = 0; inum_build < num_build.size(); ++inum_build) {
ARROW_SCOPED_TRACE("num_build = ", static_cast<int>(num_build[inum_build]));
ARROW_SCOPED_TRACE("strategy = ",
strategy[istrategy] == BloomFilterBuildStrategy::PARALLEL
? "PARALLEL"
: "SINGLE_THREADED");
ARROW_SCOPED_TRACE("avx2 = ", use_avx2 ? "AVX2" : "SCALAR");
TestBloomLarge(strategy[istrategy], num_build[inum_build], use_avx2,
std::vector<BloomFilterBuildStrategy> strategies;
strategies.push_back(BloomFilterBuildStrategy::PARALLEL);

for (const auto hardware_flags : HardwareFlagsForTesting()) {
for (const auto& strategy : strategies) {
for (const auto n : num_build) {
ARROW_SCOPED_TRACE("num_build = ", n);
ARROW_SCOPED_TRACE("strategy = ", strategy == BloomFilterBuildStrategy::PARALLEL
? "PARALLEL"
: "SINGLE_THREADED");
ARROW_SCOPED_TRACE("hardware_flags = ", hardware_flags);
TestBloomLarge(strategy, n, hardware_flags,
/*enable_prefetch=*/false);
}
}
Expand Down
9 changes: 9 additions & 0 deletions cpp/src/arrow/acero/test_util_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,18 @@
#include "arrow/testing/builder.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/testing/random.h"
#include "arrow/testing/util.h"
#include "arrow/type.h"
#include "arrow/util/async_generator.h"
#include "arrow/util/cpu_info.h"
#include "arrow/util/iterator.h"
#include "arrow/util/logging.h"
#include "arrow/util/unreachable.h"
#include "arrow/util/vector.h"

namespace arrow {

using arrow::internal::CpuInfo;
using arrow::internal::Executor;

using compute::SortKey;
Expand All @@ -62,6 +65,7 @@ using compute::Take;
namespace acero {

namespace {

void ValidateOutputImpl(const ArrayData& output) {
ASSERT_OK(::arrow::internal::ValidateArrayFull(output));
TestInitialized(output);
Expand Down Expand Up @@ -116,6 +120,11 @@ void ValidateOutput(const Datum& output) {
}
}

std::vector<int64_t> HardwareFlagsForTesting() {
// Acero currently only has AVX2 optimizations
return arrow::GetSupportedHardwareFlags({CpuInfo::AVX2});
}

namespace {

struct DummyNode : ExecNode {
Expand Down
12 changes: 7 additions & 5 deletions cpp/src/arrow/acero/test_util_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "arrow/testing/gtest_util.h"
#include "arrow/util/vector.h"

#include <cstdint>
#include <functional>
#include <random>
#include <string>
Expand All @@ -33,12 +34,14 @@
#include "arrow/util/async_generator.h"
#include "arrow/util/pcg_random.h"

namespace arrow {

namespace acero {
namespace arrow::acero {

void ValidateOutput(const Datum& output);

// Enumerate all hardware flags that can be tested on this platform
// and would lead to different code paths being tested in Acero.
std::vector<int64_t> HardwareFlagsForTesting();

using StartProducingFunc = std::function<Status(ExecNode*)>;
using StopProducingFunc = std::function<void(ExecNode*)>;

Expand Down Expand Up @@ -204,5 +207,4 @@ struct TableGenerationProperties {
Result<std::shared_ptr<Table>> MakeRandomTimeSeriesTable(
const TableGenerationProperties& properties);

} // namespace acero
} // namespace arrow
} // namespace arrow::acero
54 changes: 43 additions & 11 deletions cpp/src/arrow/compute/kernels/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,20 @@
# ----------------------------------------------------------------------
# Tests that don't require the full kernel library

# Define arrow_compute_testing object library for common test files
if(ARROW_TESTING)
add_library(arrow_compute_kernels_testing OBJECT test_util.cc)
# Even though this is still just an object library we still need to "link" our
# dependencies so that include paths are configured correctly
target_link_libraries(arrow_compute_kernels_testing ${ARROW_GTEST_GTEST})
endif()

add_arrow_test(scalar_cast_test
${ARROW_COMPUTE_TEST_ARGS}
SOURCES
scalar_cast_test.cc
test_util.cc)
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

# ----------------------------------------------------------------------
# Scalar kernels
Expand All @@ -32,25 +41,36 @@ add_arrow_compute_test(scalar_type_test
scalar_boolean_test.cc
scalar_nested_test.cc
scalar_string_test.cc
test_util.cc)
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(scalar_if_else_test SOURCES scalar_if_else_test.cc test_util.cc)
add_arrow_compute_test(scalar_if_else_test
SOURCES
scalar_if_else_test.cc
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(scalar_temporal_test SOURCES scalar_temporal_test.cc test_util.cc)
add_arrow_compute_test(scalar_temporal_test
SOURCES
scalar_temporal_test.cc
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(scalar_math_test
SOURCES
scalar_arithmetic_test.cc
scalar_compare_test.cc
scalar_round_arithmetic_test.cc
test_util.cc)
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(scalar_utility_test
SOURCES
scalar_random_test.cc
scalar_set_lookup_test.cc
scalar_validity_test.cc
test_util.cc)
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_benchmark(scalar_arithmetic_benchmark PREFIX "arrow-compute")
add_arrow_benchmark(scalar_boolean_benchmark PREFIX "arrow-compute")
Expand All @@ -75,12 +95,20 @@ add_arrow_compute_test(vector_test
vector_replace_test.cc
vector_run_end_encode_test.cc
select_k_test.cc
test_util.cc)
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(vector_sort_test SOURCES vector_sort_test.cc test_util.cc)
add_arrow_compute_test(vector_sort_test
SOURCES
vector_sort_test.cc
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_compute_test(vector_selection_test SOURCES vector_selection_test.cc
test_util.cc)
add_arrow_compute_test(vector_selection_test
SOURCES
vector_selection_test.cc
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

add_arrow_benchmark(vector_hash_benchmark PREFIX "arrow-compute")
add_arrow_benchmark(vector_sort_benchmark PREFIX "arrow-compute")
Expand All @@ -94,7 +122,11 @@ add_arrow_benchmark(vector_selection_benchmark PREFIX "arrow-compute")

# Aggregates

add_arrow_compute_test(aggregate_test SOURCES aggregate_test.cc test_util.cc)
add_arrow_compute_test(aggregate_test
SOURCES
aggregate_test.cc
EXTRA_LINK_LIBS
arrow_compute_kernels_testing)

# ----------------------------------------------------------------------
# Utilities
Expand Down
Loading

0 comments on commit e954e65

Please sign in to comment.