Skip to content

test: add multi-byte UTF-8 coverage for left/right - #24219

Closed
4ktLuffy wants to merge 1 commit into
apache:mainfrom
4ktLuffy:test/left-right-multibyte-coverage
Closed

test: add multi-byte UTF-8 coverage for left/right#24219
4ktLuffy wants to merge 1 commit into
apache:mainfrom
4ktLuffy:test/left-right-multibyte-coverage

Conversation

@4ktLuffy

Copy link
Copy Markdown

Which issue does this PR close?

  • N/A — test coverage only, no linked issue.

Rationale for this change

left and right index by Unicode scalar value rather than byte offset, but currently have no sqllogictest coverage with multi-byte input. Searching the .slt corpus, substr and strpos have non-ASCII cases; left and right have none.

left_right_byte_length takes an ASCII fast path introduced in #23762 that derives the byte offset directly from the character count. That is sound only where byte length and character count coincide, and it is guarded by string.is_ascii(); non-ASCII input falls through to the char_indices() / nth_back() path.

The two consumers of that offset behave differently if it is ever wrong. general_left_right_array slices via &str indexing, which validates char boundaries. general_left_right_view slices the underlying bytes without the &str boundary check, so an incorrect offset can produce an invalid UTF-8 view rather than the explicit boundary panic seen on StringArray.

The unicode module has had several buffer-level performance changes recently — #23762 (left/right), #23586 (pad), #22171 (translate) — and none of them touched a sqllogictest file. These cases pin the character-indexing behaviour so that a future optimisation cannot alter it without a test failing.

What changes are included in this PR?

Eight cases in functions.slt, placed alongside the existing left/right tests:

  • 2-byte (héllo), 3-byte (日本語) and 4-byte (hi🌏) inputs
  • both positive and negative n, covering the byte_offset_of_char and nth_back branches
  • Utf8View variants so general_left_right_view is exercised in addition to general_left_right_array

No source changes.

Are these changes tested?

They are tests, and I checked that they actually protect the invariant rather than just record current output: temporarily replacing the is_ascii() guard with true, so the ASCII fast path always applied, made the StringArray cases fail with

end byte index 2 is not a char boundary; it is inside '日' (bytes 0..3 of string)

and the Utf8View cases return an invalid UTF-8 result. With the guard restored, cargo test -p datafusion-sqllogictest --test sqllogictests -- functions passes.

Are there any user-facing changes?

No.

left and right index by Unicode scalar value rather than byte offset, but
have no sqllogictest coverage with multi-byte input.

left_right_byte_length takes an ASCII fast path that derives the byte offset
from the character count, guarded by string.is_ascii(); non-ASCII input falls
through to the char_indices()/nth_back() path. These cases pin that behaviour
for both string representations, including Utf8View, whose consumer slices the
underlying bytes without the &str boundary check.
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 10, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.98%. Comparing base (308e212) to head (91723dc).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24219      +/-   ##
==========================================
- Coverage   81.05%   80.98%   -0.07%     
==========================================
  Files        1106     1106              
  Lines      382268   383158     +890     
  Branches   382268   383158     +890     
==========================================
+ Hits       309864   310318     +454     
- Misses      54109    54521     +412     
- Partials    18295    18319      +24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@neilconway neilconway 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.

@4ktLuffy Thanks for working on this! There are some tests for left and right in datafusion/sqllogictest/test_files/string/string_query.slt.part. From a quick skim, it seems like those cover the new test scenarios added in this PR, but let me know if you can see any scenarios / code paths that would benefit from additional test cases.

@4ktLuffy

Copy link
Copy Markdown
Author

Thanks for the pointer — you're right, and I should have found this
before opening the PR. string_query.slt.part covers everything I added:
it's included by string.slt, large_string.slt, dictionary_utf8.slt, and
string_view.slt, so each case already runs under all four string types —
including Utf8View, which was the core of my rationale — and the
unicode_1 fixture already mixes 2-, 3-, and 4-byte code points with
negative-index cases (LEFT(unicode_1, -3), RIGHT(unicode_1, -3)).

I also checked whether literal (scalar) invocation takes a distinct
path that might warrant separate cases: it doesn't — both functions go
through make_scalar_function, which converts scalar args to 1-row
arrays before the shared implementation, so literal and column calls
converge upstream of the byte/char handling. No residual gap I can
find. Closing — thanks for the quick review.

@4ktLuffy 4ktLuffy closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants