Repository navigation
Convert RecursiveChunker to iterative and cap separator list size - #158589
Conversation
Replace the recursive chunk() method in RecursiveChunker with an iterative worklist-based approach using an explicit ArrayDeque. This makes the JVM call-stack depth constant regardless of separator count. Add a MAX_SEPARATOR_COUNT (50) validation to RecursiveChunkingSettings enforced in fromMap(), validate(), and the StreamInput constructor. The validation logic is extracted into a private static validateFields() method shared by the instance validate() and the deserialization path. PendingChunk carries ChunkOffsetAndCount so that fit chunks are emitted directly on pop without unnecessary backup chunker invocations.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @john-wagster, I've created a changelog YAML for you. |
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
@buildkite test this |
kderusso
left a comment
There was a problem hiding this comment.
Hey thanks for looping me in on this change.
One question - if we're enforcing a cap now, I think this may technically constitute a breaking change, do we need to go through that formal process here?
| public RecursiveChunkingSettings(StreamInput in) throws IOException { | ||
| maxChunkSize = in.readInt(); | ||
| separators = in.readCollectionAsList(StreamInput::readString); | ||
| validateFields(maxChunkSize, separators); |
There was a problem hiding this comment.
We need to do a version check here, or it will break BWC
There was a problem hiding this comment.
I've removed validation from existing setups. Validation now will only happen when user invokes an api so we don't need to deal with someone having added a bad separators list and nodes not talking to each other. Dealing with this seemed like a bigger headache than it was worth.
There was a problem hiding this comment.
So this is going to have some downstream impacts, because ChunkingSettingsBuilder.fromMap() will call this.
Can we please make sure we have tested and have coverage for the following use cases here that violate the new cap:
SenderService#parsePersistedConfigonGET _inference/<id>and inference in generalShardBulkInferenceActionFilterindexing into an existingsemantic_textfieldSemanticTextField- reading_sourcethat's already been indexed
There was a problem hiding this comment.
all of these paths are no longer impacted; figured it was less painful to only do user input validation instead here
|
Pinging @elastic/ml-core (Team:ML) |
@kderusso I think there's a belief that this is an overly generous max and no one would be using it currently. We've been treating similar issues like this (as there's been a lot of them lately) rather than doing the breaking change process for all of them (something that when it's that large it breaks the system). Having said that if you think it's possible for someone to hit that max of 50 separators and it be a valid usecase I can either consider increasing it or take it back to the team to validate further. Thoughts? |
|
@john-wagster Happy to defer to your team's decision on breaking changes 🙂 |
|
@john-wagster Happy to defer to your team's decision on breaking changes 🙂 @kderusso 👍 your opinion is always valued |
Replace the recursive chunk() method in RecursiveChunker with an iterative worklist-based approach using an explicit ArrayDeque. This makes the JVM call-stack depth constant regardless of separator count. Add a MAX_SEPARATOR_COUNT (50) limit to RecursiveChunkingSettings, enforced only on user-facing request paths (ES|QL CHUNK function, text_similarity_reranker chunk_rescorer, PUT/UPDATE _inference) via a new enforceRequestLimits parameter threaded through fromMap(). Persistence-read paths (parsePersistedConfig, ShardBulkInferenceAction Filter, SemanticTextField source parsing) do not enforce the limit, so existing endpoints with more than 50 separators continue to work. PendingChunk carries ChunkOffsetAndCount so that fit chunks are emitted directly on pop without unnecessary backup chunker invocations.
b635019 to
a26a226
Compare
|
Apologies I did not mean to force push there (agents sigh). |
kderusso
left a comment
There was a problem hiding this comment.
LGTM, it would be good to get a 👀 from the Inference team too
💔 Backport failed
You can use sqren/backport to manually backport by running |
…58589) (#159084) * Convert RecursiveChunker to iterative and cap separator list size Replace the recursive chunk() method in RecursiveChunker with an iterative worklist-based approach using an explicit ArrayDeque. This makes the JVM call-stack depth constant regardless of separator count. Add a MAX_SEPARATOR_COUNT (50) validation to RecursiveChunkingSettings enforced in fromMap(), validate(), and the StreamInput constructor. The validation logic is extracted into a private static validateFields() method shared by the instance validate() and the deserialization path. PendingChunk carries ChunkOffsetAndCount so that fit chunks are emitted directly on pop without unnecessary backup chunker invocations. * Update docs/changelog/158589.yaml * Update 158589.yaml
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…58589) (#159096) * Convert RecursiveChunker to iterative and cap separator list size Replace the recursive chunk() method in RecursiveChunker with an iterative worklist-based approach using an explicit ArrayDeque. This makes the JVM call-stack depth constant regardless of separator count. Add a MAX_SEPARATOR_COUNT (50) validation to RecursiveChunkingSettings enforced in fromMap(), validate(), and the StreamInput constructor. The validation logic is extracted into a private static validateFields() method shared by the instance validate() and the deserialization path. PendingChunk carries ChunkOffsetAndCount so that fit chunks are emitted directly on pop without unnecessary backup chunker invocations. * Update docs/changelog/158589.yaml * Update 158589.yaml (cherry picked from commit ea8498c) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/action/TransportUpdateInferenceModelAction.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/SenderService.java
This change improves the robustness of the recursive chunking implementation in two ways:
Iterative worklist replaces recursion in RecursiveChunker
The private
chunk()method previously used recursion indexed by the separator list position. This is replaced with an iterative loop backed by an explicitArrayDeque<PendingChunk>worklist. The JVM call-stack depth is now constant regardless of the number of separators or input size. Chunks that already fit withinmaxChunkSizeare emitted immediately on pop; only oversized chunks are pushed back for further splitting with the next separator.PendingChunkcarries the word count alongside the offset so fitness checks avoid redundant re-counting and bypass the backup chunker for chunks that already fit.Separator list size validation in RecursiveChunkingSettings
A
MAX_SEPARATOR_COUNT = 50limit is added and enforced infromMap(),validate(), and theStreamInputdeserialization constructor. This provides a reasonable upper bound on custom separator lists — the largest built-inSeparatorGroup(MARKDOWN) has 8 entries, so 50 is generous for any real-world use case. The validation logic is extracted into aprivate static validateFields()method shared by the instancevalidate()and the deserialization path to avoid thethis-escapecompiler warning.Tests
RecursiveChunkerTests: added tests for many non-matching separators completing normally, and for document-order preservation when chunks resolve at different separator depths.RecursiveChunkingSettingsTests: added tests forfromMap()rejection, boundary acceptance at the limit,validate()rejection, andStreamInputdeserialization rejection of over-limit separator lists.