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



##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, 
const std::vector<int64_
 // ----------------------------------------------------------------------
 // SparseTensorConverter for SparseCSFIndex
 
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
-  using NumericTensorType = NumericTensor<TYPE>;
-  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<DataType>& index_value_type,
                            MemoryPool* pool)
       : tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
 
-  template <typename IndexValueType>
   Status Convert() {
-    using c_index_value_type = typename IndexValueType::c_type;
-    
RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+    RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, 
tensor_.shape()));
+
+    const int index_elsize =
+        checked_cast<const IntegerType&>(*index_value_type_).bit_width() / 
CHAR_BIT;
+    const int value_elsize =
+        checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / 
CHAR_BIT;

Review comment:
       As long as the signature of the function is `int(const DataType&)` it 
sounds good to me. I don't think it should be a non-internal API because it 
does not need to do error-handling. If it's a non-static member of 
FixedWidthType, then a `checked_cast` is still necessary which seems against 
the spirit of making the code simpler




----------------------------------------------------------------
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


Reply via email to