[GitHub] [arrow] rok commented on a change in pull request #7044: ARROW-6485: [Format][C++] Support the format of a COO sparse matrix that has separated row and column indices

[GitBox <gi...@apache.org>]

rok commented on a change in pull request #7044:
URL: https://github.com/apache/arrow/pull/7044#discussion_r439226180



##########
File path: cpp/src/arrow/ipc/metadata_internal.cc
##########
@@ -882,7 +882,7 @@ Status MakeSparseTensorIndexCOO(FBB& fbb, const SparseCOOIndex& sparse_index,
                                 Offset* fb_sparse_index, size_t* num_buffers) {
   *fb_sparse_index_type = flatbuf::SparseTensorIndex::SparseTensorIndexCOO;
 
-  // We assume that the value type of indices tensor is an integer.
+  // We assume that the value type of indices tensor is an integer type.

Review comment:
       Perhaps: `// We assume that indices tensor has integer value type.` ?

##########
File path: cpp/src/arrow/ipc/metadata_internal.cc
##########
@@ -901,6 +901,35 @@ Status MakeSparseTensorIndexCOO(FBB& fbb, const SparseCOOIndex& sparse_index,
   return Status::OK();
 }
 
+Status MakeSparseTensorIndexSplitCOO(FBB& fbb, const SparseSplitCOOIndex& sparse_index,
+                                     const std::vector<BufferMetadata>& buffers,
+                                     flatbuf::SparseTensorIndex* fb_sparse_index_type,
+                                     Offset* fb_sparse_index, size_t* num_buffers) {
+  *fb_sparse_index_type = flatbuf::SparseTensorIndex::SparseTensorIndexSplitCOO;
+
+  using IntOffset = flatbuffers::Offset<flatbuf::Int>;
+  std::vector<IntOffset> indices_type_offsets;
+  for (const auto& tensor : sparse_index.indices()) {
+    // We assume that the value types of indices tensors is integer types.
+    const auto& type = checked_cast<const IntegerType&>(*tensor->type());
+    auto offset = flatbuf::CreateInt(fbb, type.bit_width(), type.is_signed());
+    indices_type_offsets.push_back(offset);
+  }
+  auto fb_indices_types = fbb.CreateVector(indices_type_offsets);
+
+  // NOTE: buffers contains ndim + 1 items: indices and a data

Review comment:
       `a data` -> `a data value`?

##########
File path: cpp/src/arrow/ipc/metadata_internal.cc
##########
@@ -1329,9 +1383,25 @@ Status GetSparseTensorMetadata(const Buffer& metadata, std::shared_ptr<DataType>
     return Status::IOError(
         "Header-type of flatbuffer-encoded Message is not SparseTensor.");
   }
+
+  const auto ndim_max = static_cast<size_t>(std::numeric_limits<int>::max());
+  if (ndim_max < sparse_tensor->shape()->size()) {
+    return Status::Invalid(
+        "The sparse tensor to be read has too much number of dimensions (",
+        sparse_tensor->shape()->size(), " is given)");

Review comment:
       How about something like `Input tensor has too many dimensions`?

##########
File path: cpp/src/arrow/ipc/metadata_internal.cc
##########
@@ -1292,6 +1326,26 @@ Status GetSparseCOOIndexMetadata(const flatbuf::SparseTensorIndexCOO* sparse_ind
   return IntFromFlatbuffer(sparse_index->indicesType(), indices_type);
 }
 
+Result<std::vector<std::shared_ptr<DataType>>> GetSparseSplitCOOIndexMetadata(
+    const flatbuf::SparseTensorIndexSplitCOO* sparse_index, const size_t ndim) {
+  const auto fb_indices_types = sparse_index->indicesTypes();
+  if (fb_indices_types->size() != ndim) {
+    return Status::Invalid(
+        "The number of indices types in a SparseSplitCOOIndex is inconsistent to the "
+        "number of dimensions");

Review comment:
       `to the` -> `with the`?




----------------------------------------------------------------
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:
users@infra.apache.org