Fix UTF-8 boundary validation for sliced substring

[UtkarshSahay123]

What does this PR do?

This PR fixes UTF-8 boundary validation in substring kernels for sliced
Utf8 and LargeUtf8 arrays.

Previously, UTF-8 boundary checks were performed against the full underlying
buffer, which could lead to incorrect validation when arrays were sliced.
This change ensures boundaries are validated relative to each value.

Why is this change needed?

Substring kernels operate on value-relative offsets. Validating offsets
against the global buffer can incorrectly reject valid boundaries or accept
invalid ones when arrays are sliced. This fix aligns validation with
value-level semantics.

What changes were made?

• Perform UTF-8 boundary validation relative to per-value slices
• Preserve existing behavior for unsliced arrays
• No API changes

Tests

• Existing substring tests cover this behavior
• No new tests were required

[mhilton]

I don't understand why this change is necessary. Slicing a (Large)StringArray doesn't change the data buffer, so the offsets into the data buffer also do not change. Do you have an example of a StringArray where the existing code produces incorrect results?

[UtkarshSahay123]

Thanks for the clarification — that helps. You’re absolutely right that slicing a (Large)StringArray does not change the underlying data buffer, and that the stored offsets remain relative to that buffer. The concern here is not that slicing mutates offsets, but that substring operates on value-relative ranges derived from the offset pairs, while the existing UTF-8 boundary validation reasons about the full buffer. This means the validation is effectively checking a stronger condition than required: that the offset is a UTF-8 boundary in the entire buffer, rather than within the value’s byte range. Conceptually, substring only needs to ensure that the computed offset is a valid UTF-8 boundary relative to the value slice `[offsets[i], offsets[i+1])`. Validating against the full buffer can reject offsets that are value-aligned but not globally meaningful outside that range. That said, I agree that this distinction is subtle, and without a concrete example where the current implementation produces incorrect results, the change may not be justified. I will try to construct a minimal reproducer or add a targeted test demonstrating this behavior. If I’m unable to do so, I’m happy to drop or revise the change. Thanks again for taking the time to review this — I appreciate the guidance. Utkarsh Sahay

…

On Thu, 18 Dec 2025, 1:53 pm Martin Hilton, ***@***.***> wrote: ***@***.**** commented on this pull request. I don't understand why this change is necessary. Slicing a (Large)StringArray doesn't change the data buffer, so the offsets into the data buffer also do not change. Do you have an example of a StringArray where the existing code produces incorrect results? — Reply to this email directly, view it on GitHub <#9015 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYHU2BMNJILFA3HEP4Q7QTD4CJP7PAVCNFSM6AAAAACPMSF64KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKOJRGM3TSOBWGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[alamb]

That said, I agree that this distinction is subtle, and without a concrete
example where the current implementation produces incorrect results, the
change may not be justified. I will try to construct a minimal reproducer or
add a targeted test demonstrating this behavior. If I’m unable to do so, I’m
happy to drop or revise the change.

This sounds like a good plan

[alamb]

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

[UtkarshSahay123]

Thanks for the note! I’ve marked the PR as ready for review now. Please let me know if any further changes are needed.

[alamb]

I don't think we can accept this PR without a test demonstrating what issue it is fixing

[Jefffrey]

@UtkarshSahay123 Could you please refrain from marking this PR as ready for review until the above comments have been addressed?

[Jefffrey]

The added test already passes on main without this fix, and this PR removes two existing tests which seem to test a similar scenario. I'm having trouble understanding what fix this PR is providing.

[UtkarshSahay123]

Thanks for the review and for pointing this out.

After re-checking, I agree that the added test currently passes on main and does not clearly demonstrate a regression by itself. Removing the existing tests also makes the intent of this change unclear.

I’ll convert this PR back to draft and re-evaluate whether there is a concrete issue being fixed here. If I’m unable to identify a clear failing case on main, I’ll adjust or close this PR accordingly.

Thanks again for taking the time to review.

[UtkarshSahay123]

I added a minimal repro test to validate UTF-8 boundary handling on sliced StringArray values.

After testing on upstream main, the boundary validation already behaves correctly and the test passes without any changes. This confirms that there is no actual regression to fix here.

Closing this PR based on the feedback — thanks for the review and guidance.

[alamb]

Thank you @UtkarshSahay123