Skip to content

Fix incorrect string tensor creation in test - #29219

Merged
yuslepukhin merged 2 commits into
mainfrom
yuslepukhin/gh_issue_13246_string_tensor
Jun 23, 2026
Merged

Fix incorrect string tensor creation in test#29219
yuslepukhin merged 2 commits into
mainfrom
yuslepukhin/gh_issue_13246_string_tensor

Conversation

@yuslepukhin

Copy link
Copy Markdown
Contributor

This pull request refactors the CreateGetVectorOfMapsStringFloat test in test_nontensor_types.cc to improve type safety and clarity by switching from std::vector to std::array for fixed-size data and updating tensor creation logic. The changes also fix the string comparison to use the correct array.

Test improvements and type safety:

  • Replaced std::vector with std::array for dims and values variables, making the code safer and more efficient for fixed-size data.
  • Updated the creation of the string tensor by using Ort::AllocatorWithDefaultOptions() and filling the tensor with FillStringTensor, aligning with best practices for string tensor creation.

Test correctness:

  • Fixed the assertion to compare against keys_arr instead of the undefined keys vector, ensuring the test checks the correct set of keys.

Header inclusion:

  • Added #include <array> at the top of the file to support the use of std::array.

…oat test

The test was passing std::string* as raw memory to CreateTensor, which
is invalid for string tensors. Use CreateTensor with allocator +
FillStringTensor instead (matching the pattern in CreateGetSeqStringTensors).

Also remove redundant keys vector and use std::array for fixed-size dims
and values.

Fixes #13246

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the CreateGetVectorOfMapsStringFloat shared-lib C++ API test by switching to the correct mechanism for creating and populating a string tensor (allocate as ONNX_TENSOR_ELEMENT_DATA_TYPE_STRING and then fill via FillStringTensor), and cleans up fixed-size test data by using std::array.

Changes:

  • Use std::array for the fixed-size shape (dims) and float payload (values).
  • Correct string tensor creation by allocating a string tensor via Ort::AllocatorWithDefaultOptions() and populating it with Value::FillStringTensor.
  • Fix the final key-set assertion to compare against keys_arr (the actual source of truth).

@hariharans29

Copy link
Copy Markdown
Member

Review: PR #29219 — Fix incorrect string tensor creation in test

Verdict: Approve. Clean, correct bug fix for issue #13246.

What it does

The pre-fix code created a STRING tensor by passing the raw bytes of a std::vector<std::string> as if they were the tensor's data buffer:

Ort::Value::CreateTensor(info, keys.data(), keys.size() * sizeof(std::string),
                         dims.data(), dims.size(), ONNX_TENSOR_ELEMENT_DATA_TYPE_STRING);

That overload is for tensors that wrap a caller-owned numeric buffer. String tensors in ORT don't store std::string objects — they have their own internal string storage that must be allocated by ORT and populated via FillStringTensor. So the original test was almost certainly UB (interpreting std::string headers / heap pointers as string-tensor payload), which is exactly what issue #13246 reported.

The fix uses the correct two-step API:

Ort::Value::CreateTensor(Ort::AllocatorWithDefaultOptions(), dims.data(), dims.size(),
                         ONNX_TENSOR_ELEMENT_DATA_TYPE_STRING);   // ORT-owned string storage
keys_tensor.FillStringTensor(keys_arr, NUM_KV_PAIRS);             // populate the strings

That's the canonical pattern (matches everywhere else in the codebase that creates STRING tensors via the C++ API).

Other changes

  • std::vector<int64_t> dimsstd::array<int64_t, 1> dims: idiomatic for compile-time-sized data, no behavior change (.data() / .size() still match what CreateTensor expects). Fine.
  • std::vector<float> valuesstd::array<float, NUM_KV_PAIRS> values: same; the values_tensor line below still works (values.size() * sizeof(float) = 16, correct). Fine.
  • ASSERT_EQ(keys_ret, std::setstd::string(std::begin(keys), std::end(keys))) → ... std::begin(keys_arr), std::end(keys_arr) ...: required follow-on since the keys vector is gone, and it's also the assertion-correctness fix the description calls out (keys_arr is the actual source of truth — keys was just a copy of it, so behavior is unchanged but the dependency is now direct).
  • #include <array>: required for the type change. Correct.

Non-blocking observations

  • Ort::AllocatorWithDefaultOptions() is constructed inside the loop body. It's a thin wrapper around a singleton, so this is harmless — wouldn't ask to refactor.
  • The float values_tensor still uses the wrap-external-buffer overload (info, values.data(), values.size() * sizeof(float), ...). That's correct for non-string types, so the asymmetry with the string tensor is intentional and right. Don't unify them.

CI

22 / 73 checks OK at time of review. Safe to approve now; merge once CI fully clears.

@yuslepukhin
yuslepukhin merged commit 6e9596e into main Jun 23, 2026
86 checks passed
@yuslepukhin
yuslepukhin deleted the yuslepukhin/gh_issue_13246_string_tensor branch June 23, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_nontensor_types.cc incorrectly creates string tensor -- invaliding the test

3 participants