Skip to content

fix: burrows_wheeler_transform round-trip corruption and unicode panic - #1064

Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
TheAlgorithms:masterfrom
SEPURI-SAI-KRISHNA:fix/bwt-roundtrip-corruption
Open

fix: burrows_wheeler_transform round-trip corruption and unicode panic#1064
SEPURI-SAI-KRISHNA wants to merge 1 commit into
TheAlgorithms:masterfrom
SEPURI-SAI-KRISHNA:fix/bwt-roundtrip-corruption

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown

Pull Request Template

Description

burrows_wheeler_transform and inv_burrows_wheeler_transform sort by two different
orderings, so the transform is not reversible for any input that mixes upper and lower
case
. No error is raised — the inverse simply returns a different string:

let encoded = burrows_wheeler_transform("Hello");        // ("Hoell", 1)
inv_burrows_wheeler_transform(encoded);                  // "elloe"  -- expected "Hello"

inv_burrows_wheeler_transform(burrows_wheeler_transform("Mississippi"));
// "issippMissi" -- expected "Mississippi"

Why it happens

Inverting a BWT works only when the forward rotation sort and the inverse's character sort
are the same total order. They were not:

  • the forward transform sorted rotations with sort_by_key(|a| a.to_lowercase()), i.e.
    case-insensitively;
  • the inverse sorted the encoded characters with sort_by_key(|a| a.1), i.e. by code point.

For "Hello" the two disagree immediately:

forward sort (to_lowercase): ["elloH", "Hello", "lloHe", "loHel", "oHell"]
plain lexicographic sort:    ["Hello", "elloH", "lloHe", "loHel", "oHell"]
inverse sorts chars as:      ['H', 'e', 'l', 'l', 'o']   // 'H' = 72 before 'e' = 101

Dropping .to_lowercase() makes the forward sort plain lexicographic. Rust compares String
bytewise and UTF-8 byte order matches code-point order, so the forward sort now agrees with
the inverse's char sort exactly. The existing expectations are unaffected because every
test string was single-case ("CARROT", "THEALGORITHMS") or punctuation, where
to_lowercase() is a no-op on the ordering.

Rotations were also built by slicing at byte offsets

input[i..].to_owned() + &input[..i] indexes a &str by an arbitrary i, which panics as
soon as the index lands inside a multi-byte character:

burrows_wheeler_transform("café au lait");
// panicked: byte index 4 is not a char boundary; it is inside 'é' (bytes 3..5)

Rotations are now built from a Vec<char>, so every index is a character position.
inv_burrows_wheeler_transform got the matching treatment: it used input.len() (a byte
count) to drive a loop that indexed by character, and called chars().nth(i) inside that
loop, which is a quadratic scan. Both are gone.

The stability of sort_by_key in the inverse is what keeps equal characters in their original
relative order, which the reconstruction walk depends on; that is now stated in a comment so
it does not get "optimised" into sort_unstable_by_key later.

Tests

Added mixed_case (7 strings that all round-tripped to garbage before), unicode (6 strings
that all panicked before), single_character and repeated_characters. The existing tests
are unchanged and still pass. All run in well under 300ms.

Note: src/compression/burrows_wheeler_transform.rs is a separate implementation and does
not have this bug — it sorts consistently in both directions. It is untouched here.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I ran bellow commands using the latest version of rust nightly.
  • I ran cargo clippy --all -- -D warnings just before my last commit and fixed any issue that was found.
  • I ran cargo fmt just before my last commit.
  • I ran cargo test just before my last commit and all tests passed.
  • I added my algorithm to the corresponding mod.rs file within its own folder, and in any parent folder(s).
  • I added my algorithm to DIRECTORY.md with the correct link.
  • I checked COUNTRIBUTING.md and my code follows its guidelines.

Note: no mod.rs or DIRECTORY.md change was needed — this fixes an existing entry rather
than adding a new algorithm.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.89%. Comparing base (2c53ddf) to head (3e0a263).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1064   +/-   ##
=======================================
  Coverage   95.89%   95.89%           
=======================================
  Files         396      396           
  Lines       30440    30473   +33     
=======================================
+ Hits        29190    29222   +32     
- Misses       1250     1251    +1     

☔ 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.

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.

2 participants