The logic regarding separator retention in RecursiveCharacterTextSplitter partitioning can be problematic

[xiuxiuxius]

Checked other resources

• I added a very descriptive title to this question.
• I searched the LangChain documentation with the integrated search.
• I used the GitHub search to find a similar question and didn't find it.

Commit to Help

• I commit to help with one of those options 👆

Example Code

def _split_text(self, text: str, separators: List[str]) -> List[str]:
        """Split incoming text and return chunks."""
        final_chunks = []
        # Get appropriate separator to use
        separator = separators[-1]
        new_separators = []
        for i, _s in enumerate(separators):
            _separator = _s if self._is_separator_regex else re.escape(_s)
            if _s == "":
                separator = _s
                break
            if re.search(_separator, text):
                separator = _s
                new_separators = separators[i + 1 :]
                break

        _separator = separator if self._is_separator_regex else re.escape(separator)
        splits = _split_text_with_regex(text, _separator, self._keep_separator)

        # Now go merging things, recursively splitting longer texts.
        _good_splits = []
        _separator = "" if self._keep_separator else separator
        for s in splits:
            if self._length_function(s) < self._chunk_size:
                _good_splits.append(s)
            else:
                if _good_splits:
                    merged_text = self._merge_splits(_good_splits, _separator)
                    final_chunks.extend(merged_text)
                    _good_splits = []
                if not new_separators:
                    final_chunks.append(s)
                else:
                    other_info = self._split_text(s, new_separators)
                    final_chunks.extend(other_info)
        if _good_splits:
            merged_text = self._merge_splits(_good_splits, _separator)
            final_chunks.extend(merged_text)
        return final_chunks

Description

I think this sentence in the code:

_separator = "" if self._keep_separator else separator

If the separator needs to be retained, then '_separator' should equal the current separator. So the logic should be reversed:

_separator = separator if self._keep_separator else ""

This preserves the current separator when 'self._keep_separator' is set to 'True'.

System Info

System Information

OS: Linux
OS Version: #1 SMP Tue Jun 4 14:43:51 UTC 2024
Python Version: 3.10.14 (main, May 6 2024, 19:42:50) [GCC 11.2.0]

Package Information

langchain_core: 0.2.28
langchain: 0.2.7
langchain_community: 0.2.7
langsmith: 0.1.86
langchain_openai: 0.1.20
langchain_text_splitters: 0.2.2
langgraph: 0.2.3

Packages not installed (Not Necessarily a Problem)

The following packages were not found:

langserve

[dosubot[bot]]

Hey @xiuxiuxius! I'm here to help you with your bug, answer questions, and even guide you on becoming a contributor. Let's dive into the issue you're facing!

To address the issue with separator retention in the RecursiveCharacterTextSplitter's _split_text method, you are correct that the logic for _separator should be adjusted. Here is the corrected implementation:

def _split_text(self, text: str, separators: List[str]) -> List[str]:
    """Split incoming text and return chunks."""
    final_chunks = []
    # Get appropriate separator to use
    separator = separators[-1]
    new_separators = []
    for i, _s in enumerate(separators):
        _separator = _s if self._is_separator_regex else re.escape(_s)
        if _s == "":
            separator = _s
            break
        if re.search(_separator, text):
            separator = _s
            new_separators = separators[i + 1 :]
            break

    _separator = separator if self._is_separator_regex else re.escape(separator)
    splits = _split_text_with_regex(text, _separator, self._keep_separator)

    # Now go merging things, recursively splitting longer texts.
    _good_splits = []
    _separator = separator if self._keep_separator else ""
    for s in splits:
        if self._length_function(s) < self._chunk_size:
            _good_splits.append(s)
        else:
            if _good_splits:
                merged_text = self._merge_splits(_good_splits, _separator)
                final_chunks.extend(merged_text)
                _good_splits = []
            if not new_separators:
                final_chunks.append(s)
            else:
                other_info = self._split_text(s, new_separators)
                final_chunks.extend(other_info)
    if _good_splits:
        merged_text = self._merge_splits(_good_splits, _separator)
        final_chunks.extend(merged_text)
    return final_chunks

This change ensures that _separator correctly retains the current separator when self._keep_separator is set to True ^[1].

To continue talking to Dosu, mention @dosu.

[xiuxiuxius]

Oh, I see _split_text_with_regex Here, if self._keep_separator is True, then the separator is already reserved, not the join

def _split_text_with_regex(
    text: str, separator: str, keep_separator: Union[bool, Literal["start", "end"]]
) -> List[str]:
    # Now that we have the separator, split the text
    if separator:
        if keep_separator:
            # The parentheses in the pattern keep the delimiters in the result.
            _splits = re.split(f"({separator})", text)
            splits = (
                ([_splits[i] + _splits[i + 1] for i in range(0, len(_splits) - 1, 2)])
                if keep_separator == "end"
                else ([_splits[i] + _splits[i + 1] for i in range(1, len(_splits), 2)])
            )
            if len(_splits) % 2 == 0:
                splits += _splits[-1:]
            splits = (
                (splits + [_splits[-1]])
                if keep_separator == "end"
                else ([_splits[0]] + splits)
            )
        else:
            splits = re.split(separator, text)
    else:
        splits = list(text)
    return [s for s in splits if s != ""]