From 6a14b96f87467c123ee5f6a6329248febac39b6a Mon Sep 17 00:00:00 2001 From: Kenta Murata Date: Mon, 24 Jun 2019 16:47:35 +0900 Subject: [PATCH 1/4] Stop using ARROW_TEMPLATE_EXPORT for SparseTensorImpl --- cpp/src/arrow/sparse_tensor.cc | 201 +++++++++++++-------------------- cpp/src/arrow/sparse_tensor.h | 23 +++- 2 files changed, 97 insertions(+), 127 deletions(-) diff --git a/cpp/src/arrow/sparse_tensor.cc b/cpp/src/arrow/sparse_tensor.cc index 6b4f99e6368..6bc1d772365 100644 --- a/cpp/src/arrow/sparse_tensor.cc +++ b/cpp/src/arrow/sparse_tensor.cc @@ -135,18 +135,6 @@ class SparseTensorConverter using BaseClass::tensor_; }; -template -void MakeSparseTensorFromTensor(const Tensor& tensor, - std::shared_ptr* sparse_index, - std::shared_ptr* data) { - NumericTensor numeric_tensor(tensor.data(), tensor.shape(), tensor.strides()); - SparseTensorConverter converter(numeric_tensor); - Status s = converter.Convert(); - DCHECK_OK(s); - *sparse_index = converter.sparse_index; - *data = converter.data; -} - // ---------------------------------------------------------------------- // SparseTensorConverter for SparseCSRIndex @@ -244,6 +232,86 @@ INSTANTIATE_SPARSE_TENSOR_CONVERTER(SparseCSRIndex); } // namespace +namespace internal { + +namespace { + +template +Status MakeSparseTensorFromTensor(const Tensor& tensor, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { + NumericTensor numeric_tensor(tensor.data(), tensor.shape(), tensor.strides()); + SparseTensorConverter converter(numeric_tensor); + Status s = converter.Convert(); + RETURN_NOT_OK(s); + *sparse_index = converter.sparse_index; + *data = converter.data; + return Status::OK(); +} + +template +inline Status MakeSparseTensorFromTensor(const Tensor& tensor, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { + switch (tensor.type()->id()) { + case Type::UINT8: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::INT8: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::UINT16: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::INT16: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::UINT32: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::INT32: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::UINT64: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::INT64: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::HALF_FLOAT: + return MakeSparseTensorFromTensor( + tensor, sparse_index, data); + case Type::FLOAT: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + case Type::DOUBLE: + return MakeSparseTensorFromTensor(tensor, sparse_index, + data); + default: + return Status::NotImplemented("Unspported Tensor value type"); + } +} + +} // namespace + +Status MakeSparseTensorFromTensor(const Tensor& tensor, + SparseTensorFormat::type sparse_format_id, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { + switch (sparse_format_id) { + case SparseTensorFormat::COO: + return MakeSparseTensorFromTensor(tensor, sparse_index, data); + + case SparseTensorFormat::CSR: + return MakeSparseTensorFromTensor(tensor, sparse_index, data); + + default: + return Status::Invalid("Invalid sparse tensor format ID"); + } +} + +} // namespace internal + // ---------------------------------------------------------------------- // SparseCOOIndex @@ -303,113 +371,4 @@ bool SparseTensor::Equals(const SparseTensor& other) const { return SparseTensorEquals(*this, other); } -// ---------------------------------------------------------------------- -// SparseTensorImpl - -// Constructor with a dense tensor -template -SparseTensorImpl::SparseTensorImpl( - const std::shared_ptr& type, const std::vector& shape, - const std::vector& dim_names) - : SparseTensorImpl(nullptr, type, nullptr, shape, dim_names) {} - -// Constructor with a dense tensor -template -template -SparseTensorImpl::SparseTensorImpl(const NumericTensor& tensor) - : SparseTensorImpl(nullptr, tensor.type(), nullptr, tensor.shape(), - tensor.dim_names_) { - SparseTensorConverter converter(tensor); - Status s = converter.Convert(); - DCHECK_OK(s); - sparse_index_ = converter.sparse_index; - data_ = converter.data; -} - -// Constructor with a dense tensor -template -SparseTensorImpl::SparseTensorImpl(const Tensor& tensor) - : SparseTensorImpl(nullptr, tensor.type(), nullptr, tensor.shape(), - tensor.dim_names_) { - switch (tensor.type()->id()) { - case Type::UINT8: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::INT8: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::UINT16: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::INT16: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::UINT32: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::INT32: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::UINT64: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::INT64: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::HALF_FLOAT: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::FLOAT: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - case Type::DOUBLE: - MakeSparseTensorFromTensor(tensor, &sparse_index_, - &data_); - return; - default: - break; - } -} - -// ---------------------------------------------------------------------- -// Instantiate templates - -#define INSTANTIATE_SPARSE_TENSOR(IndexType) \ - template class ARROW_TEMPLATE_EXPORT SparseTensorImpl; \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&); \ - template ARROW_EXPORT SparseTensorImpl::SparseTensorImpl( \ - const NumericTensor&) - -INSTANTIATE_SPARSE_TENSOR(SparseCOOIndex); -INSTANTIATE_SPARSE_TENSOR(SparseCSRIndex); - } // namespace arrow diff --git a/cpp/src/arrow/sparse_tensor.h b/cpp/src/arrow/sparse_tensor.h index e622245d633..1e78f7aee07 100644 --- a/cpp/src/arrow/sparse_tensor.h +++ b/cpp/src/arrow/sparse_tensor.h @@ -217,6 +217,15 @@ class ARROW_EXPORT SparseTensor { // ---------------------------------------------------------------------- // SparseTensorImpl class +namespace internal { + +Status MakeSparseTensorFromTensor(const Tensor& tensor, + SparseTensorFormat::type sparse_format_id, + std::shared_ptr* sparse_index, + std::shared_ptr* data); + +} + /// \brief EXPERIMENTAL: Concrete sparse tensor implementation classes with sparse index /// type template @@ -234,14 +243,16 @@ class ARROW_EXPORT SparseTensorImpl : public SparseTensor { // Constructor for empty sparse tensor SparseTensorImpl(const std::shared_ptr& type, const std::vector& shape, - const std::vector& dim_names = {}); - - // Constructor with a dense numeric tensor - template - explicit SparseTensorImpl(const NumericTensor& tensor); + const std::vector& dim_names = {}) + : SparseTensorImpl(nullptr, type, nullptr, shape, dim_names) {} // Constructor with a dense tensor - explicit SparseTensorImpl(const Tensor& tensor); + explicit SparseTensorImpl(const Tensor& tensor) + : SparseTensorImpl(nullptr, tensor.type(), nullptr, tensor.shape(), + tensor.dim_names_) { + (void)internal::MakeSparseTensorFromTensor(tensor, SparseIndexType::format_id, + &sparse_index_, &data_); + } private: ARROW_DISALLOW_COPY_AND_ASSIGN(SparseTensorImpl); From 5a6468fe7f4a14f49354493b6a0b4cea65368c59 Mon Sep 17 00:00:00 2001 From: Kenta Murata Date: Tue, 25 Jun 2019 10:41:50 +0900 Subject: [PATCH 2/4] Replace nullptr with NULLPTR --- cpp/src/arrow/sparse_tensor.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/sparse_tensor.h b/cpp/src/arrow/sparse_tensor.h index 1e78f7aee07..9a0f64e2a14 100644 --- a/cpp/src/arrow/sparse_tensor.h +++ b/cpp/src/arrow/sparse_tensor.h @@ -244,11 +244,11 @@ class ARROW_EXPORT SparseTensorImpl : public SparseTensor { SparseTensorImpl(const std::shared_ptr& type, const std::vector& shape, const std::vector& dim_names = {}) - : SparseTensorImpl(nullptr, type, nullptr, shape, dim_names) {} + : SparseTensorImpl(NULLPTR, type, NULLPTR, shape, dim_names) {} // Constructor with a dense tensor explicit SparseTensorImpl(const Tensor& tensor) - : SparseTensorImpl(nullptr, tensor.type(), nullptr, tensor.shape(), + : SparseTensorImpl(NULLPTR, tensor.type(), NULLPTR, tensor.shape(), tensor.dim_names_) { (void)internal::MakeSparseTensorFromTensor(tensor, SparseIndexType::format_id, &sparse_index_, &data_); From 819cce53ee5a5ab0778c3715226f01ce0a02478a Mon Sep 17 00:00:00 2001 From: Kenta Murata Date: Tue, 25 Jun 2019 18:37:39 +0900 Subject: [PATCH 3/4] Remove ARROW_EXPORT from SparseTensorImpl --- cpp/src/arrow/sparse_tensor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/sparse_tensor.h b/cpp/src/arrow/sparse_tensor.h index 9a0f64e2a14..396bb2a2a50 100644 --- a/cpp/src/arrow/sparse_tensor.h +++ b/cpp/src/arrow/sparse_tensor.h @@ -229,7 +229,7 @@ Status MakeSparseTensorFromTensor(const Tensor& tensor, /// \brief EXPERIMENTAL: Concrete sparse tensor implementation classes with sparse index /// type template -class ARROW_EXPORT SparseTensorImpl : public SparseTensor { +class SparseTensorImpl : public SparseTensor { public: virtual ~SparseTensorImpl() = default; From a24f78c944ab388435395feb71a6fae47ba531ad Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Tue, 25 Jun 2019 19:38:23 -0500 Subject: [PATCH 4/4] Code review feedback --- cpp/src/arrow/sparse_tensor.cc | 83 +++++++++++++++++---------------- cpp/src/arrow/sparse_tensor.h | 15 +++--- cpp/src/arrow/util/visibility.h | 10 ---- 3 files changed, 50 insertions(+), 58 deletions(-) diff --git a/cpp/src/arrow/sparse_tensor.cc b/cpp/src/arrow/sparse_tensor.cc index 6bc1d772365..fc2d5386cb6 100644 --- a/cpp/src/arrow/sparse_tensor.cc +++ b/cpp/src/arrow/sparse_tensor.cc @@ -237,76 +237,77 @@ namespace internal { namespace { template -Status MakeSparseTensorFromTensor(const Tensor& tensor, - std::shared_ptr* sparse_index, - std::shared_ptr* data) { +void MakeSparseTensorFromTensor(const Tensor& tensor, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { NumericTensor numeric_tensor(tensor.data(), tensor.shape(), tensor.strides()); SparseTensorConverter converter(numeric_tensor); - Status s = converter.Convert(); - RETURN_NOT_OK(s); + ARROW_CHECK_OK(converter.Convert()); *sparse_index = converter.sparse_index; *data = converter.data; - return Status::OK(); } template -inline Status MakeSparseTensorFromTensor(const Tensor& tensor, - std::shared_ptr* sparse_index, - std::shared_ptr* data) { +inline void MakeSparseTensorFromTensor(const Tensor& tensor, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { switch (tensor.type()->id()) { case Type::UINT8: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::INT8: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::UINT16: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::INT16: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::UINT32: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::INT32: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::UINT64: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::INT64: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::HALF_FLOAT: - return MakeSparseTensorFromTensor( - tensor, sparse_index, data); + MakeSparseTensorFromTensor(tensor, sparse_index, + data); + break; case Type::FLOAT: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case Type::DOUBLE: - return MakeSparseTensorFromTensor(tensor, sparse_index, - data); + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; default: - return Status::NotImplemented("Unspported Tensor value type"); + ARROW_LOG(FATAL) << "Unsupported Tensor value type"; + break; } } } // namespace -Status MakeSparseTensorFromTensor(const Tensor& tensor, - SparseTensorFormat::type sparse_format_id, - std::shared_ptr* sparse_index, - std::shared_ptr* data) { +void MakeSparseTensorFromTensor(const Tensor& tensor, + SparseTensorFormat::type sparse_format_id, + std::shared_ptr* sparse_index, + std::shared_ptr* data) { switch (sparse_format_id) { case SparseTensorFormat::COO: - return MakeSparseTensorFromTensor(tensor, sparse_index, data); - + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; case SparseTensorFormat::CSR: - return MakeSparseTensorFromTensor(tensor, sparse_index, data); - + MakeSparseTensorFromTensor(tensor, sparse_index, data); + break; default: - return Status::Invalid("Invalid sparse tensor format ID"); + ARROW_LOG(FATAL) << "Invalid sparse tensor format ID"; + break; } } diff --git a/cpp/src/arrow/sparse_tensor.h b/cpp/src/arrow/sparse_tensor.h index 396bb2a2a50..b6fe4b20597 100644 --- a/cpp/src/arrow/sparse_tensor.h +++ b/cpp/src/arrow/sparse_tensor.h @@ -219,12 +219,13 @@ class ARROW_EXPORT SparseTensor { namespace internal { -Status MakeSparseTensorFromTensor(const Tensor& tensor, - SparseTensorFormat::type sparse_format_id, - std::shared_ptr* sparse_index, - std::shared_ptr* data); +ARROW_EXPORT +void MakeSparseTensorFromTensor(const Tensor& tensor, + SparseTensorFormat::type sparse_format_id, + std::shared_ptr* sparse_index, + std::shared_ptr* data); -} +} // namespace internal /// \brief EXPERIMENTAL: Concrete sparse tensor implementation classes with sparse index /// type @@ -250,8 +251,8 @@ class SparseTensorImpl : public SparseTensor { explicit SparseTensorImpl(const Tensor& tensor) : SparseTensorImpl(NULLPTR, tensor.type(), NULLPTR, tensor.shape(), tensor.dim_names_) { - (void)internal::MakeSparseTensorFromTensor(tensor, SparseIndexType::format_id, - &sparse_index_, &data_); + internal::MakeSparseTensorFromTensor(tensor, SparseIndexType::format_id, + &sparse_index_, &data_); } private: diff --git a/cpp/src/arrow/util/visibility.h b/cpp/src/arrow/util/visibility.h index b224717a62d..95cd9cf5ba2 100644 --- a/cpp/src/arrow/util/visibility.h +++ b/cpp/src/arrow/util/visibility.h @@ -43,14 +43,4 @@ #endif #endif // Non-Windows -// This is a complicated topic, some reading on it: -// http://www.codesynthesis.com/~boris/blog/2010/01/18/dll-export-cxx-templates/ -#if defined(_MSC_VER) || defined(__clang__) -#define ARROW_TEMPLATE_CLASS_EXPORT -#define ARROW_TEMPLATE_EXPORT ARROW_EXPORT -#else -#define ARROW_TEMPLATE_CLASS_EXPORT ARROW_EXPORT -#define ARROW_TEMPLATE_EXPORT -#endif - #endif // ARROW_UTIL_VISIBILITY_H