[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module

2020-06-29 Thread GitBox


mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447268862



##
File path: cpp/src/arrow/tensor/csf_converter.cc
##
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector& coord, 
const std::vector
-class SparseCSFTensorConverter {
- public:
-  using NumericTensorType = NumericTensor;
-  using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+  using SparseTensorConverterMixin::AssignIndex;
+  using SparseTensorConverterMixin::IsNonZero;
 
-  SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+  SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr& index_value_type,
MemoryPool* pool)
   : tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
 
-  template 
   Status Convert() {
-using c_index_value_type = typename IndexValueType::c_type;
-
RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits::max()));
+RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, 
tensor_.shape()));
+
+const int index_elsize =
+checked_cast(*index_value_type_).bit_width() / 
CHAR_BIT;
+const int value_elsize =
+checked_cast(*tensor_.type()).bit_width() / 
CHAR_BIT;

Review comment:
   Oops, I don't need to return `-1` because `checked_cast` is used in this 
function.  Sorry.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module

2020-06-29 Thread GitBox


mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447267538



##
File path: cpp/src/arrow/tensor/csf_converter.cc
##
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector& coord, 
const std::vector
-class SparseCSFTensorConverter {
- public:
-  using NumericTensorType = NumericTensor;
-  using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+  using SparseTensorConverterMixin::AssignIndex;
+  using SparseTensorConverterMixin::IsNonZero;
 
-  SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+  SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr& index_value_type,
MemoryPool* pool)
   : tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
 
-  template 
   Status Convert() {
-using c_index_value_type = typename IndexValueType::c_type;
-
RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits::max()));
+RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, 
tensor_.shape()));
+
+const int index_elsize =
+checked_cast(*index_value_type_).bit_width() / 
CHAR_BIT;
+const int value_elsize =
+checked_cast(*tensor_.type()).bit_width() / 
CHAR_BIT;

Review comment:
   OK. I'll make `inernal::GetByteWidth`, and let it return `-1` for 
non-`FixedWidthType` values.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module

2020-06-29 Thread GitBox


mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447259489



##
File path: cpp/src/arrow/tensor/csf_converter.cc
##
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector& coord, 
const std::vector
-class SparseCSFTensorConverter {
- public:
-  using NumericTensorType = NumericTensor;
-  using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+  using SparseTensorConverterMixin::AssignIndex;
+  using SparseTensorConverterMixin::IsNonZero;
 
-  SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+  SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr& index_value_type,
MemoryPool* pool)
   : tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
 
-  template 
   Status Convert() {
-using c_index_value_type = typename IndexValueType::c_type;
-
RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits::max()));
+RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, 
tensor_.shape()));
+
+const int index_elsize =
+checked_cast(*index_value_type_).bit_width() / 
CHAR_BIT;
+const int value_elsize =
+checked_cast(*tensor_.type()).bit_width() / 
CHAR_BIT;

Review comment:
   I agreed with you. I felt it is better to extract this idiom as a 
function, too.
   `internal::GetByteWith` looks good to me, but how about the member function 
of `FixedWidthType`?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org