dkp116 opened a new pull request, #50500:
URL: https://github.com/apache/arrow/pull/50500
### Rationale for this change
Investigated the two usages of `DeleteMany` in the codebase to verify
whether there could be an issue caused by incorrect or duplicate indexes being
passed.
There are two locations where `DeleteMany` is called:
1. `cpp/src/arrow/c/bridge.cc`
```cpp
DeleteMany(metadata_.extension_name_index,
metadata_.extension_serialized_index)
```
The indexes are populated while iterating through metadata keys:
```cpp
if (keys[i] == kExtensionTypeKeyName) {
decoded.extension_name = values[i];
decoded.extension_name_index = i;
} else if (keys[i] == kExtensionMetadataKeyName) {
decoded.extension_serialized = values[i];
decoded.extension_serialized_index = i;
}
```
Here due to the if and else if statement there is not a case where they are
the same value.
2. `cpp/src/arrow/ipc/metadata_internal.cc`
```cpp
metadata->DeleteMany({name_index, data_index});
```
Here, the indexes are obtained using:
```cpp
name_index = metadata->FindKey(kExtensionTypeKeyName);
data_index = metadata->FindKey(kExtensionMetadataKeyName);
```
`FindKey` traverses the `keys_` array maintained by `KeyValueMetadata` and
returns the index of the matching key. Since the two searched keys are
different:
`cpp/src/arrow/extension_type.cc`
```cpp
extern const char kExtensionTypeKeyName[] = "ARROW:extension:name";
extern const char kExtensionMetadataKeyName[] = "ARROW:extension:metadata";
```
the returned indexes refer to two different metadata entries.
### What changes are included in this PR?
This change documents/investigates the behavior of the existing
`DeleteMany`, and a log is produced when DeleteMany is accessing an invalid
memory address instead of throwing with this error:
/usr/include/c++/16.1.1/bits/stl_vector.h:1253: constexpr std::vector<_Tp,
_Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp =
std::__cxx11::basic_string<char>; _Alloc =
std::allocator<std::__cxx11::basic_string<char> >;
reference = std::__cxx11::basic_string<char>&; size_type = long unsigned
int]: Assertion '__n < this->size()' failed.
Note: I have not prevented the use of the same index in DeleteMany as for
example DeleteMany(1,1) will still give an output, given a valid key and array
vector so no functionality has been changed, just a more readable log has been
added.
### Are these changes tested?
A test has been added to `cpp/src/arrow/util/key_value_metadata_test.cc`
which checks to see the correct error is shown when an invalid memory address
is being accessed
### Are there any user-facing changes?
No user-facing changes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
