[C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format

Describe the bug, including details regarding any error messages, version, and platform.

In the following schema file, it is assumed that there is a single type for all indptr buffers and another single type for all indices buffers. However, in the C++ implementation, each indptr and indices buffer can have its own type.

Schema files

arrow/format/SparseTensor.fbs

Line 160 in d803afc

indptrType: Int (required);

arrow/format/SparseTensor.fbs

Line 179 in d803afc

indicesType: Int (required);

C++ implementation
Note that each tensor is associated with its own type:

arrow/cpp/src/arrow/sparse_tensor.h

Lines 445 to 447 in d803afc

    
           protected: 
        
            std::vector<std::shared_ptr<Tensor>> indptr_; 
        
            std::vector<std::shared_ptr<Tensor>> indices_;

In the C++ IPC implementation, however, the type of the first indices and indptr buffer is generalized to all:

arrow/cpp/src/arrow/ipc/metadata_internal.cc

Lines 1152 to 1153 in d803afc

    
           const auto& indptr_value_type = 
        
               checked_cast<const IntegerType&>(*sparse_index.indptr()[0]->type());

arrow/cpp/src/arrow/ipc/metadata_internal.cc

Lines 1158 to 1159 in d803afc

    
           const auto& indices_value_type = 
        
               checked_cast<const IntegerType&>(*sparse_index.indices()[0]->type());

Component(s)

C++, Documentation, Other

@rok

@rok I’m not familiar with FlatBuffers at all, so I may be misunderstanding. Could you confirm whether this is correct?

I would read

arrow/format/SparseTensor.fbs

Line 160 in d803afc

indptrType: Int (required);

and

arrow/format/SparseTensor.fbs

Line 179 in d803afc

indicesType: Int (required);

as: indptrType is a required 8-64bit integer type and indicesType is a required 8-64bit integer type. (Note Int maps to integer while int means 32 bit integer). I don't see anywhere stated that these are the same integer type, so C++ implementation seems consistent with the flatbuffers definition. See Int definition:

arrow/format/Schema.fbs

Lines 159 to 162 in d803afc

    
           table Int { 
        
             bitWidth: int; // restricted to 8, 16, 32, and 64 in v1 
        
             is_signed: bool; 
        
           }

That said it might make sense that the two are always the same type. Would you like to discuss that?

I mean that the same integer type is assumed for every indptr buffer in the C++ IPC implementation, but in reality, each indptr buffer can have its own integer type. For example, one indptr buffer could be int32, while another could be int64. However, in the C++ IPC implementation, only a single integer type is considered for all indptr buffers:
C++implementation

arrow/cpp/src/arrow/ipc/metadata_internal.cc

Lines 1169 to 1175 in d803afc

    
           for (const std::shared_ptr<arrow::Tensor>& tensor : sparse_index.indptr()) { 
        
             const int64_t size = tensor->data()->size() / indptr_elem_size; 
        
             const int64_t padded_size = PaddedLength(tensor->data()->size(), kArrowIpcAlignment); 
        
             indptr.push_back({offset, size}); 
        
             offset += padded_size; 
        
           }

This happens because only one type is maintained for all indptr buffers. However, each buffer actually has its own type. The same issue also exists for the indices buffers.

The above explanation may be unclear. I provide a C++ implementation example to demonstrate that the .fbs schema for SparseCSFIndex is invalid.

I see, you're saying buffers in indptrBuffers: [Buffer] (required); [1] could have different types? I don't think generated flatbuffers deserializer would provide that option. Also - mixing types here would not make much sense application-wise.

Also - mixing types here would not make much sense application-wise.

I have no idea about this.
However, I think it could be a potential bug that may cause illegal memory access

@andishgar

I'm looking at the SparseCSFIndex::Make and it will make std::vector<std::shared_ptr<Buffer>>& indptr_data with a single integer type. However one could construct a CSFIndex directly with different integer types as you're suggesting. We could add a check for this in CheckSparseCSFIndexValidity so constructor and SparseCSFIndex::Make would throw an error. Thanks for noticing and explaining @andishgar !

@rok

@rok, your suggestion to enforce using the same type is fine. However, this module has existed for more than five years. Would it be safe to change its behavior now? I’m concerned about potential compatibility issues. (I’m aware that Arrow has a major release every three months and that backward compatibility is not guaranteed for major releases—I just want to be sure.)

@andishgar

@andishgar In my view indptr and indices were designed to contain only one datatype and this wouldn't be a change in behavior, just a stricter validity check. Buffers containing different datatypes read through arrow-cpp would return bad data or trigger segfaults. So I don't think adding an extra validity check would be an issue.
We have (to my knowledge) never heard from any CSF index users. Are you aware of anyone using it in the wild?

I haven’t heard of CSF with different indptr types. Actually, I’m only concerned with memory access. :-).
So, in summary, we should enforce—and document—that there is only one type for all indptr buffers and another single type for all index buffers

That seems like the thing to do. Something like this would be needed for enforcement:

diff --git i/cpp/src/arrow/sparse_tensor.cc w/cpp/src/arrow/sparse_tensor.cc
index b84070b3d2..9e191ef794 100644
--- i/cpp/src/arrow/sparse_tensor.cc
+++ w/cpp/src/arrow/sparse_tensor.cc
@@ -397,6 +397,12 @@ SparseCSFIndex::SparseCSFIndex(const std::vector<std::shared_ptr<Tensor>>& indpt
                                const std::vector<std::shared_ptr<Tensor>>& indices,
                                const std::vector<int64_t>& axis_order)
     : SparseIndexBase(), indptr_(indptr), indices_(indices), axis_order_(axis_order) {
+  for (const auto & tensor : indptr_) {
+    ARROW_CHECK_EQ(indptr_.front()->type(), tensor->type());
+  }
+  for (const auto & tensor : indices_) {
+    ARROW_CHECK_EQ(indices_.front()->type(), tensor->type());
+  }
   ARROW_CHECK_OK(CheckSparseCSFIndexValidity(indptr_.front()->type(),
                                              indices_.front()->type(), indptr_.size(),
                                              indices_.size(), axis_order_.size()));

This issue will be resolved in #47586.

Navigation Menu

Repository navigation

[C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format #47613

Describe the bug, including details regarding any error messages, version, and platform.

Component(s)

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	protected:
	std::vector<std::shared_ptr<Tensor>> indptr_;
	std::vector<std::shared_ptr<Tensor>> indices_;

	const auto& indptr_value_type =
	checked_cast<const IntegerType&>(*sparse_index.indptr()[0]->type());

	const auto& indices_value_type =
	checked_cast<const IntegerType&>(*sparse_index.indices()[0]->type());

	table Int {
	bitWidth: int; // restricted to 8, 16, 32, and 64 in v1
	is_signed: bool;
	}

	for (const std::shared_ptr<arrow::Tensor>& tensor : sparse_index.indptr()) {
	const int64_t size = tensor->data()->size() / indptr_elem_size;
	const int64_t padded_size = PaddedLength(tensor->data()->size(), kArrowIpcAlignment);

	indptr.push_back({offset, size});
	offset += padded_size;
	}

[C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format #47613

Description

Describe the bug, including details regarding any error messages, version, and platform.

Component(s)

Activity

andishgar commented on Sep 22, 2025

rok commented on Sep 22, 2025

andishgar commented on Sep 22, 2025

andishgar commented on Sep 22, 2025

rok commented on Sep 22, 2025

andishgar commented on Sep 22, 2025

rok commented on Sep 22, 2025

andishgar commented on Sep 22, 2025

rok commented on Sep 22, 2025

andishgar commented on Sep 22, 2025

rok commented on Sep 22, 2025

andishgar commented on Oct 8, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions