Skip to content

ARROW-4124: [C++] Draft Aggregate and Sum kernels#3407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fsaintjacques wants to merge 14 commits into from
Closed

ARROW-4124: [C++] Draft Aggregate and Sum kernels #3407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3e9451e
ARROW-4124: Implement Aggregate and Sum kernels
fsaintjacques Jan 15, 2019
92a4e03
Fix
fsaintjacques Jan 15, 2019
f1044be
Fix warnings
fsaintjacques Jan 23, 2019
c4b5b21
Fix lint issues.
fsaintjacques Jan 23, 2019
440ed3d
Doxygen error
fsaintjacques Jan 24, 2019
d5ea4c8
Refactor
fsaintjacques Feb 7, 2019
0ff3bae
lint and format
fsaintjacques Feb 7, 2019
cadc709
Remove mutex header
fsaintjacques Feb 7, 2019
f062737
Use bigger width type for accumulator.
fsaintjacques Feb 8, 2019
9bd1f25
Warnings
fsaintjacques Feb 8, 2019
27d32b5
Use multiplication method with sentinel benchmarks
wesm Feb 10, 2019
985e9ce
Rename arrow-compute-sum-benchmark/test
wesm Feb 10, 2019
c272059
Some code review comments
wesm Feb 10, 2019
568ba09
Add braces
wesm Feb 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cpp/src/arrow/CMakeLists.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -130,9 +130,11 @@ if (ARROW_COMPUTE)
add_subdirectory(compute)
set(ARROW_SRCS ${ARROW_SRCS}
compute/context.cc
compute/kernels/aggregate.cc
compute/kernels/boolean.cc
compute/kernels/cast.cc
compute/kernels/hash.cc
compute/kernels/sum.cc
compute/kernels/util-internal.cc
)
endif()
Expand Down
53 changes: 49 additions & 4 deletions cpp/src/arrow/compute/kernel.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -57,25 +57,66 @@ class ARROW_EXPORT OpKernel {

/// \brief Placeholder for Scalar values until we implement these
struct ARROW_EXPORT Scalar {
~Scalar() {}
util::variant<bool, uint8_t, int8_t, uint16_t, int16_t, uint32_t, int32_t, uint64_t,
int64_t, float, double>
value;

@wesmwesmJan 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I'm not sure about this. See https://issues.apache.org/jira/browse/ARROW-47. It's OK to leave this as is but marked experimental

@wesmwesmFeb 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file feel like a kludge to me. Let's not do anything else involving scalar outputs until resolving ARROW-47. I'll put up a patch for that tomorrow with any luck


ARROW_DISALLOW_COPY_AND_ASSIGN(Scalar);
explicit Scalar(bool value) : value(value) {}
explicit Scalar(uint8_t value) : value(value) {}
explicit Scalar(int8_t value) : value(value) {}
explicit Scalar(uint16_t value) : value(value) {}
explicit Scalar(int16_t value) : value(value) {}
explicit Scalar(uint32_t value) : value(value) {}
explicit Scalar(int32_t value) : value(value) {}
explicit Scalar(uint64_t value) : value(value) {}
explicit Scalar(int64_t value) : value(value) {}
explicit Scalar(float value) : value(value) {}
explicit Scalar(double value) : value(value) {}

Type::type kind() const {
switch (this->value.which()) {
case 0:

@bkietzbkietzJan 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you just static_cast<Type::type>(this->value.which())?

@fsaintjacquesfsaintjacquesJan 16, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd have to make sure the mapping is exact, which is almost equivalent to expliciting this list.

return Type::BOOL;
case 1:
return Type::UINT8;
case 2:
return Type::INT8;
case 3:
return Type::UINT16;
case 4:
return Type::INT16;
case 5:
return Type::UINT32;
case 6:
return Type::INT32;
case 7:
return Type::UINT64;
case 8:
return Type::INT64;
case 9:
return Type::FLOAT;
case 10:
return Type::DOUBLE;
default:
return Type::NA;
}
}
};

/// \class Datum
/// \brief Variant type for various Arrow C++ data structures
struct ARROW_EXPORT Datum {
enum type { NONE, SCALAR, ARRAY, CHUNKED_ARRAY, RECORD_BATCH, TABLE, COLLECTION };

util::variant<decltype(NULLPTR), std::shared_ptr<Scalar>, std::shared_ptr<ArrayData>,
util::variant<decltype(NULLPTR), Scalar, std::shared_ptr<ArrayData>,
std::shared_ptr<ChunkedArray>, std::shared_ptr<RecordBatch>,
std::shared_ptr<Table>, std::vector<Datum>>
value;

/// \brief Empty datum, to be populated elsewhere
Datum() : value(NULLPTR) {}

Datum(const std::shared_ptr<Scalar>& value) // NOLINT implicit conversion
Datum(const Scalar& value) // NOLINT implicit conversion
: value(value) {}
Datum(const std::shared_ptr<ArrayData>& value) // NOLINT implicit conversion
: value(value) {}
Expand DownExpand Up@@ -147,6 +188,10 @@ struct ARROW_EXPORT Datum {
return util::get<std::vector<Datum>>(this->value);
}

Scalar scalar() const { return util::get<Scalar>(this->value); }

bool is_array() const { return this->kind() == Datum::ARRAY; }

bool is_arraylike() const {
return this->kind() == Datum::ARRAY || this->kind() == Datum::CHUNKED_ARRAY;
}
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/arrow/compute/kernels/CMakeLists.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,3 +20,7 @@ ARROW_INSTALL_ALL_HEADERS("arrow/compute/kernels")
ADD_ARROW_TEST(boolean-test PREFIX "arrow-compute")
ADD_ARROW_TEST(cast-test PREFIX "arrow-compute")
ADD_ARROW_TEST(hash-test PREFIX "arrow-compute")

# Aggregates
ADD_ARROW_TEST(aggregate-test PREFIX "arrow-compute")
ADD_ARROW_BENCHMARK(aggregate-benchmark PREFIX "arrow-compute")
Loading