You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "andishgar (via GitHub)" <gi...@apache.org> on 2025/09/22 08:32:10 UTC
[I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
andishgar opened a new issue, #47613:
URL: https://github.com/apache/arrow/issues/47613
### 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**
[https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L160](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L160)
[https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L179](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L179)
**C++ implementation**
Note that each tensor is associated with its own type:
[https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse\_tensor.h#L445-L447](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse_tensor.h#L445-L447)
In the C++ IPC implementation, however, the type of the first `indices` and `indptr` buffer is generalized to all:
[https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/ipc/metadata\_internal.cc#L1152-L1153](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/ipc/metadata_internal.cc#L1152-L1153)
[https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/ipc/metadata\_internal.cc#L1158-L1159](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/ipc/metadata_internal.cc#L1158-L1159)
### Component(s)
C++, Documentation, Other
--
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.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318589150
@andishgar In my view `indptr` and `indices` this was designed to contain only one datatype and this wouldn't be a change in behavior, just a stricter validity check. Buffers containing different datatype 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?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar closed issue #47613: [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format
URL: https://github.com/apache/arrow/issues/47613
--
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.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318646250
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
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318790245
That seems like the thing to do. Something like this would be needed for enforcement:
```diff
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 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318194375
I'm looking at the [`SparseCSFIndex::Make`](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse_tensor.cc#L364) 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` ](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse_tensor.cc#L340) so [constructor](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse_tensor.cc#L396) and [`SparseCSFIndex::Make`](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/sparse_tensor.cc#L364) would throw an error. Thanks for noticing and explaining @andishgar !
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3317570617
@rok I’m not familiar with FlatBuffers at all, so I may be misunderstanding. Could you confirm whether this is correct?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3317912631
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
https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/cpp/src/arrow/ipc/metadata_internal.cc#L1169-L1175
This happens because only one type is maintained for all [indptr buffers](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L160). However, each buffer actually has its own type. The same issue also exists for the `indices` buffers.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318046804
>Also - mixing types here would not make much sense application-wise.
I really have no idea about this.
However, I think it could be a potential bug that may cause illegal memory access
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3381972430
This issue will be resolved in #47520.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3318316094
@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.)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3317759576
I would read https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L160 and
https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L179
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:
https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/Schema.fbs#L159-L162
That said it might make sense that the two are always the same type. Would you like to discuss that?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3317993294
I see, you're saying buffers in `indptrBuffers: [Buffer] (required);` [[1]](https://github.com/apache/arrow/blob/d803afcc43f5d132506318fd9e162d33b2c3d4cd/format/SparseTensor.fbs#L175) 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.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [C++][IPC][Tensor] Incorrect SparseTensorIndexCSF IPC format [arrow]
Posted by "andishgar (via GitHub)" <gi...@apache.org>.
andishgar commented on issue #47613:
URL: https://github.com/apache/arrow/issues/47613#issuecomment-3317983769
The above explanation may be unclear. I provide a C++ implementation example to demonstrate that the `.fbs schema `for `SparseCSFIndex `is invalid.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org