Skip to content

Commit 5b8bd1b

Browse files
fsecada01claude
andcommitted
Fix 10 code review findings from Rust/PyO3 core review
Correctness fixes: - token.rs: implement distinct truncate_smart strategy (2:1 head/tail weighting) — was identical to middle strategy - chunk.rs: always flush on max_tokens overflow; remove min_tokens guard that silently allowed chunks to exceed the hard cap - chunk.rs: fix find_table_end CRLF handling — advance by actual terminator byte count (2 for CRLF, 1 for LF) instead of always +1 - encoding.rs: detect UTF-8 BOM before chardetng so decode uses utf-8-sig (strips BOM) rather than utf-8 (preserves it) - normalize.rs: seed strip_headers_footers candidates from all pages, not just page 0, so cover-page documents strip running headers Fallback path fixes (_fallback.py): - detect_encoding: try cp1252 before latin-1 so Windows smart-quote bytes (0x80-0x9F) decode to printable chars, not C1 control chars - chunk(): oversized paragraphs now set metadata={oversized: True} to match the Rust interface contract - chunk(): use re.split capturing group to track actual separator lengths; fixes char_start/char_end drift after 3+-newline gaps - count/truncate/chunk._count: pass allowed_special=all to tiktoken encode() to match Rust encode_with_special_tokens behaviour core.py: - _decode_bytes chain updated to (utf-8, cp1252, latin-1) so text/CSV files correctly handle Windows-encoded content Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 130df1e commit 5b8bd1b

7 files changed

Lines changed: 131 additions & 50 deletions

File tree

TextSpitter/_fallback.py

Lines changed: 64 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ def detect_encoding(data: bytes) -> str:
1515
"""Detect encoding by trying common codecs in priority order."""
1616
if not data:
1717
return "utf-8"
18-
for enc in ("utf-8", "utf-8-sig", "latin-1", "cp1252"):
18+
for enc in ("utf-8", "utf-8-sig", "cp1252", "latin-1"):
1919
try:
2020
data.decode(enc)
2121
return enc
@@ -150,24 +150,74 @@ def _count(self, text: str) -> int:
150150
import tiktoken
151151

152152
enc = tiktoken.get_encoding(self.tokenizer)
153-
return len(enc.encode(text))
153+
# Mirror Rust encode_with_special_tokens: allow all special tokens.
154+
return len(enc.encode(text, allowed_special="all"))
154155
except Exception:
155-
# Rough character-based fallback: ~4 chars per token
156156
return len(text) // 4
157157

158158
def chunk(self, text: str) -> list[Chunk]:
159159
import re
160160

161-
paragraphs = [p.strip() for p in re.split(r"\n\n+", text) if p.strip()]
161+
# Split with a capturing group so we can measure the actual separator
162+
# length (2+ newlines). Without this, char_cursor drifts when gaps use
163+
# 3+ newlines because the old code always added a fixed +2.
164+
pieces = re.split(r"(\n\n+)", text)
162165
chunks: list[Chunk] = []
163166
current_parts: list[str] = []
164167
current_tokens = 0
165168
char_cursor = 0
166169
current_start = 0
167170
section_title: str | None = None
168171

169-
for para in paragraphs:
172+
for idx, piece in enumerate(pieces):
173+
if idx % 2 == 1:
174+
# Odd indices are separator strings ("\n\n", "\n\n\n", …)
175+
char_cursor += len(piece)
176+
continue
177+
178+
para = piece.strip()
179+
if not para:
180+
char_cursor += len(piece)
181+
continue
182+
170183
para_tokens = self._count(para)
184+
185+
# Single paragraph exceeds max_tokens — emit as oversized.
186+
if para_tokens > self.max_tokens:
187+
if current_parts:
188+
chunk_text = "\n\n".join(current_parts)
189+
chunks.append(
190+
Chunk(
191+
text=chunk_text,
192+
token_count=current_tokens,
193+
char_start=current_start,
194+
char_end=char_cursor,
195+
section_title=section_title,
196+
chunk_index=0,
197+
total_chunks=None,
198+
metadata={},
199+
)
200+
)
201+
current_parts = []
202+
current_tokens = 0
203+
current_start = char_cursor
204+
end = char_cursor + len(piece)
205+
chunks.append(
206+
Chunk(
207+
text=para,
208+
token_count=para_tokens,
209+
char_start=char_cursor,
210+
char_end=end,
211+
section_title=section_title,
212+
chunk_index=0,
213+
total_chunks=None,
214+
metadata={"oversized": True},
215+
)
216+
)
217+
char_cursor = end
218+
current_start = char_cursor
219+
continue
220+
171221
if current_tokens + para_tokens > self.max_tokens and current_parts:
172222
chunk_text = "\n\n".join(current_parts)
173223
chunks.append(
@@ -188,7 +238,7 @@ def chunk(self, text: str) -> list[Chunk]:
188238

189239
current_parts.append(para)
190240
current_tokens += para_tokens
191-
char_cursor += len(para) + 2 # +2 for \n\n
241+
char_cursor += len(piece)
192242

193243
if current_parts:
194244
chunk_text = "\n\n".join(current_parts)
@@ -235,7 +285,13 @@ def _bpe(self):
235285

236286
def count(self, text: str) -> int:
237287
bpe = self._bpe()
238-
return len(bpe.encode(text)) if bpe else len(text) // 4
288+
# allowed_special="all" mirrors Rust encode_with_special_tokens and
289+
# prevents ValueError when text contains tokens like <|endoftext|>.
290+
return (
291+
len(bpe.encode(text, allowed_special="all"))
292+
if bpe
293+
else len(text) // 4
294+
)
239295

240296
def count_batch(self, texts: list[str]) -> list[int]:
241297
return [self.count(t) for t in texts]
@@ -255,7 +311,7 @@ def truncate(
255311
else:
256312
kept = words[:max_tokens]
257313
return " ".join(kept)
258-
tokens = bpe.encode(text)
314+
tokens = bpe.encode(text, allowed_special="all")
259315
if len(tokens) <= max_tokens:
260316
return text
261317
if strategy == "middle":

TextSpitter/core.py

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -400,8 +400,12 @@ def docx_file_read(self) -> str: # Added return type hint
400400

401401
def _decode_bytes(self, data: bytes, label: str) -> str:
402402
"""
403-
Decode bytes to str, trying UTF-8 then latin-1 then UTF-8 with
404-
replacement characters.
403+
Decode bytes to str, trying UTF-8, cp1252, then latin-1, then UTF-8
404+
with replacement characters.
405+
406+
cp1252 is tried before latin-1 so Windows smart-quote bytes (0x80-0x9F)
407+
decode to printable characters instead of C1 control characters.
408+
latin-1 always succeeds and acts as the final deterministic fallback.
405409
406410
Args:
407411
data: Raw bytes to decode.
@@ -410,16 +414,13 @@ def _decode_bytes(self, data: bytes, label: str) -> str:
410414
Returns:
411415
str
412416
"""
413-
try:
414-
return data.decode("utf-8")
415-
except UnicodeDecodeError:
416-
pass
417-
try:
418-
return data.decode("latin-1")
419-
except UnicodeDecodeError:
420-
pass
417+
for enc in ("utf-8", "cp1252", "latin-1"):
418+
try:
419+
return data.decode(enc)
420+
except (UnicodeDecodeError, LookupError):
421+
continue
421422
logger.warning(
422-
f"Could not decode {label} with utf-8 or latin-1, "
423+
f"Could not decode {label} with utf-8, cp1252, or latin-1, "
423424
f"using utf-8 with replacement characters."
424425
)
425426
return data.decode("utf-8", errors="replace")

src/chunk.rs

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -189,21 +189,19 @@ impl TextChunker {
189189

190190
let pending_tokens = bpe.encode_with_special_tokens(&current_text).len();
191191

192+
// Always flush on overflow — max_tokens is a hard cap; min_tokens
193+
// is a soft target that must not allow chunks to exceed max_tokens.
192194
if pending_tokens + unit_tokens > self.max_tokens && !current_text.is_empty() {
193-
// Emit pending chunk if it meets min_tokens.
194-
let pending_count = bpe.encode_with_special_tokens(&current_text).len();
195-
if pending_count >= self.min_tokens || chunks.is_empty() {
196-
chunks.push(self.make_chunk(
197-
&current_text,
198-
&bpe,
199-
current_start,
200-
char_cursor,
201-
current_section.clone(),
202-
false,
203-
));
204-
current_text.clear();
205-
current_start = char_cursor;
206-
}
195+
chunks.push(self.make_chunk(
196+
&current_text,
197+
&bpe,
198+
current_start,
199+
char_cursor,
200+
current_section.clone(),
201+
false,
202+
));
203+
current_text.clear();
204+
current_start = char_cursor;
207205
}
208206

209207
if let Some(title) = &unit.section_title {
@@ -338,15 +336,20 @@ fn push_text_units(
338336

339337
fn find_table_end(text: &str, start: usize) -> usize {
340338
let from = &text[start..];
341-
let mut end = start;
339+
let mut offset = 0usize;
342340
for line in from.lines() {
343-
if line.trim_start().starts_with('|') {
344-
end += line.len() + 1; // +1 for newline
345-
} else if line.trim().is_empty() {
346-
end += line.len() + 1;
341+
if line.trim_start().starts_with('|') || line.trim().is_empty() {
342+
offset += line.len();
343+
// lines() strips line terminators; advance past the actual bytes
344+
// so CRLF (2 bytes) is handled correctly, not just LF (1 byte).
345+
if from[offset..].starts_with("\r\n") {
346+
offset += 2;
347+
} else if offset < from.len() {
348+
offset += 1;
349+
}
347350
} else {
348351
break;
349352
}
350353
}
351-
end.min(text.len())
354+
(start + offset).min(text.len())
352355
}

src/encoding.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@ pub fn detect_encoding(data: &[u8]) -> String {
2525
return "utf-8".into();
2626
}
2727

28+
// Explicit BOM check before chardetng: chardetng returns "UTF-8" for
29+
// BOM-prefixed files, but Python's "utf-8" codec preserves the BOM at
30+
// position 0. "utf-8-sig" strips it during decode.
31+
if data.starts_with(b"\xef\xbb\xbf") {
32+
return "utf-8-sig".into();
33+
}
34+
2835
// Feed the entire buffer; last=true signals end-of-stream.
2936
let mut detector = EncodingDetector::new();
3037
detector.feed(data, true);

src/normalize.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,20 +82,25 @@ fn strip_headers_footers(text: &str) -> String {
8282
.map(|p| p.lines().collect())
8383
.collect();
8484

85-
let candidate_lines: std::collections::HashSet<&str> = all_lines[0]
86-
.iter()
85+
// Collect every unique non-empty line from all pages, then keep only those
86+
// present on more than half the pages. Seeding from page 0 alone misses
87+
// running headers when page 0 is a cover page with no shared lines.
88+
let all_unique: std::collections::HashSet<&str> = all_lines.iter()
89+
.flat_map(|pg| pg.iter().copied())
90+
.filter(|l| !l.trim().is_empty())
91+
.collect();
92+
93+
let candidate_lines: std::collections::HashSet<&str> = all_unique
94+
.into_iter()
8795
.filter(|line| {
8896
let trimmed = line.trim();
89-
if trimmed.is_empty() { return false; }
9097
let count = all_lines.iter()
9198
.filter(|page_lines| {
9299
page_lines.iter().any(|l| l.trim() == trimmed)
93100
})
94101
.count();
95-
// Present on more than half the pages = header/footer
96102
count * 2 > pages.len()
97103
})
98-
.copied()
99104
.collect();
100105

101106
if candidate_lines.is_empty() {

src/token.rs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,18 @@ impl TokenCounter {
9494
}
9595

9696
fn truncate_smart(tokens: &[usize], max_tokens: usize) -> Vec<usize> {
97-
let keep_start = max_tokens / 2;
97+
// Weight the head 2:1 over the tail — beginning of document carries more
98+
// context; middle is dropped first, then tail is trimmed before head.
99+
let n = tokens.len();
100+
let keep_start = (max_tokens * 2).div_ceil(3);
98101
let keep_end = max_tokens - keep_start;
99-
let mut result = tokens[..keep_start].to_vec();
100-
result.extend_from_slice(&tokens[tokens.len() - keep_end..]);
101-
result
102+
let tail_start = n.saturating_sub(keep_end);
103+
104+
if keep_end == 0 || tail_start <= keep_start {
105+
tokens[..max_tokens.min(n)].to_vec()
106+
} else {
107+
let mut result = tokens[..keep_start].to_vec();
108+
result.extend_from_slice(&tokens[tail_start..]);
109+
result
110+
}
102111
}

tests/test_file_extractor.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ def mock_decode_side_effect(encoding, errors=None):
456456
result = extractor.text_file_read()
457457

458458
assert (
459-
"Could not decode text file badtext.txt with utf-8 or latin-1"
459+
"Could not decode text file badtext.txt with utf-8, cp1252, or latin-1"
460460
in "\n".join(log_capture)
461461
)
462462
mock_bytes_instance.decode.assert_any_call("utf-8", errors="replace")
@@ -506,7 +506,7 @@ def mock_decode_side_effect(encoding, errors=None):
506506
result = extractor.csv_file_read()
507507

508508
assert (
509-
"Could not decode CSV file bad.csv with utf-8 or latin-1"
509+
"Could not decode CSV file bad.csv with utf-8, cp1252, or latin-1"
510510
in "\n".join(log_capture)
511511
)
512512
mock_bytes_instance.decode.assert_any_call("utf-8", errors="replace")

0 commit comments

Comments
 (0)