CAMEL-23953: Add batch operations for langchain4j embeddings - #25273
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
several files require a regen
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/langchain4j-embeddings.json
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/langchain4j-embeddingstore.json
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/langchain4j-embeddings-component.adoc
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/langchain4j-embeddingstore-component.adoc
modified: dsl/camel-endpointdsl/src/generated/java/org/apache/camel/builder/endpoint/dsl/LangChain4jEmbeddingStoreEndpointBuilderFactory.java
modified: dsl/camel-endpointdsl/src/generated/java/org/apache/camel/builder/endpoint/dsl/LangChain4jEmbeddingsEndpointBuilderFactory.java
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 568 tested, 24 compile-only — current: 565 all testedMaveniverse Scalpel detected 592 affected modules (current approach: 565).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The batch operations design is well-structured and addresses a real performance concern for RAG pipelines. Tests are well-written using AssertJ and a recording store pattern, and documentation with Java/YAML examples is good. However, there are several issues that should be addressed:
[HIGH] Destructive removeAll() fallback — The remove() method now calls store.removeAll() (clearing the entire store) when body is null/empty and no filter is set. Previously this would throw IllegalArgumentException via langchain4j's ensureNotBlank() — a safe failure mode. Making the most destructive operation the default fallback when no input is provided is a data safety risk. Consider requiring explicit intent (e.g., a dedicated header flag or a distinct action like CLEAR).
[MEDIUM] Caller-supplied IDs silently discarded in addBatch() — When EMBEDDING_IDS header is set but no TextSegment body is provided, the code falls through to store.addAll(embeddings) which generates new IDs, silently ignoring the user-provided IDs. Should either loop with add(id, embedding) or throw an error explaining that caller-supplied IDs require text segments for batch operations.
[MEDIUM] FQCN for ArrayList — new java.util.ArrayList<>() used without importing ArrayList, violating the project's no-FQCN convention.
[MEDIUM] EMBEDDINGS header not in CamelLangchain4jAttributes — The new cross-component header constant uses a hardcoded string instead of referencing CamelLangchain4jAttributes in core/camel-api, breaking consistency with the existing EMBEDDING, VECTOR, TEXT_SEGMENT pattern.
[LOW] Missing upgrade guide entry — No entry in the upgrade guide for the behavioral change in REMOVE operation (null body previously threw exception, now clears the store).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
Claude Code on behalf of gnodet All review findings have been addressed in commit f267c76:
Regarding the upgrade guide point: since the REMOVE-with-null-body behavior never existed in a released version (it was introduced in this PR's first commit and corrected in this follow-up), there's no behavioral change for existing users to migrate from — so no upgrade guide entry is needed. |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thorough batch-operations implementation for both embedding and embedding-store components. The caller-supplied-ID support and batch REMOVE are welcome additions. A few data-flow issues stood out that could affect RAG ingestion pipelines:
1. Text segments lost in batch embed → store pipeline (medium)
In LangChain4jEmbeddingsProducer.processBatch(), the message body is overwritten with List<Embedding> at line 96 (message.setBody(embeddings)). Unlike the single-document path — which preserves the original text segment via the TEXT_SEGMENT header — the batch path sets no TEXT_SEGMENTS header. When this flows downstream to the embedding store producer, addBatch() checks whether the body is List<TextSegment> but finds List<Embedding> instead, falling through to addAll(embeddings) and storing embeddings without their text segments.
This is the exact chaining pipeline documented in the PR's own langchain4j-embeddingstore-component.adoc (embed → store), making it a real data-loss scenario for RAG pipelines — search results will return embeddings but not the original text.
2. Size mismatch between callerIds and embeddings not validated (medium)
In LangChain4jEmbeddingStoreProducer.addBatch(), when callerIds != null && textSegments == null, the loop iterates embeddings.size() times and accesses callerIds.get(i) without a size-equality check. If callerIds.size() < embeddings.size(), this throws an unhelpful IndexOutOfBoundsException. If callerIds.size() > embeddings.size(), trailing IDs are silently ignored and incorrectly included in the response body. A defensive size check with a clear error message would improve debuggability.
3. Caller-supplied ID path silently drops text segment (medium)
In the single-add path, when callerId != null, store.add(callerId, embedding) is called and the text-segment check is skipped entirely. The langchain4j EmbeddingStore API has no add(String id, Embedding, TextSegment) method, which explains the current code. However, addAll(List<String>, List<Embedding>, List<TextSegment>) exists and could be used with singleton lists as a workaround. The PR's documentation example shows a chain that sets EMBEDDING_ID after embedding — this exact pipeline loses the text segment.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
f267c76 to
a3c3ecb
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for this well-structured addition of batch operations to the langchain4j embedding components — the RecordingEmbeddingStore test pattern is particularly good, and the documentation with Java/YAML tabs is solid.
Two medium-severity issues remain from the prior self-review rounds that should be addressed before merging.
This review focuses on project rules and conventions. It does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
Grok code review — CAMEL-23953Verdict: Approve with improvements — focused, well-tested batch wiring that closes the LangChain4j API gap without breaking existing routes. ArchitectureClean dispatch; good javadoc on operation matrix. Strengths
Remaining suggestions
Issue status summary
RecommendationReady for merge after upgrade-guide entry and list-size validation (small diff). Suggest reviewers: @gnodet (author), @davsclaus (AI components). AI-generated Grok review on behalf of atiaomar1978-hub. |
Bugbot review — CAMEL-23953 batch embeddings/embeddingstoreReviewed ScopeAdds batch Verified (looks good)
Open issues (see inline threads)
Test coverage verdictGood for merge — core batch paths and regression guard ( Bugbot verdict: Approve with minor follow-ups (upgrade guide + list validation). AI-generated Bugbot review on behalf of atiaomar1978-hub. |
apupier
left a comment
apupier
left a comment
There was a problem hiding this comment.
several conflicts and build is failing
Add batch embedAll, addAll, removeAll support for LangChain4j embedding components, enabling efficient RAG ingestion pipelines. Embeddings producer: - Detect List body and route to processBatch with embedAll - Preserve text segments via TEXT_SEGMENTS header for downstream use - Full GenAI observation support for batch operations Embedding store producer: - Batch ADD via EMBEDDINGS header with addAll variants - Caller-supplied IDs via EMBEDDING_ID/EMBEDDING_IDS headers - Single add with caller ID + text segment preserved via addAll singleton - Size validation between callerIds, embeddings, and textSegments - Batch REMOVE by Collection<String> body or Filter header - Null/empty REMOVE throws IllegalArgumentException (safe failure) Shared constants: - Add CAMEL_LANGCHAIN4J_EMBEDDINGS and CAMEL_LANGCHAIN4J_TEXT_SEGMENTS to CamelLangchain4jAttributes in core/camel-api Documentation and upgrade guide: - Document batch ADD, caller-supplied IDs, and batch REMOVE - Add upgrade guide entry for REMOVE behavior change and new headers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a3c3ecb to
83b9dc9
Compare
gnodet
changed the title
|
All review findings have been addressed in the rebased commit (83b9dc9):
Claude Code on behalf of @gnodet |
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this — the batch add/remove logic is clean and the store-side test coverage (asserting which store overload each path invokes) is genuinely thorough. The prior review feedback (destructive removeAll() fallback, text-segment preservation, size validation, shared header constants) has clearly been incorporated. One blocking item and a few suggestions.
🔴 Blocking
Wrong release version targeted. camel-4.22.0 is already released and main is now 4.23.0-SNAPSHOT, so these features ship in 4.23, not 4.22:
- The two
@since 4.22tags inCamelLangchain4jAttributes.javashould be@since 4.23(CLAUDE.md: use the upcoming minor release). See inline suggestions. - The upgrade-guide entries were added to
camel-4x-upgrade-guide-4_22.adoc(a released line). They belong in the existingcamel-4x-upgrade-guide-4_23.adoc, under "Upgrading Camel 4.22 to 4.23". As-is they are attached to a version that can never contain the change. Both the=== camel-langchain4j-embeddingsand the=== camel-langchain4j-embeddingstoreadditions need to move.
🟡 Suggestions (non-blocking)
LangChain4jEmbeddingsBatchTest.batchEmbeddingTokenUsageis named/@DisplayName'd "sets token usage headers" but only assertsEMBEDDINGSis not null — it never checksINPUT_TOKEN_COUNT/TOTAL_TOKEN_COUNT. Either assert the token headers or rename the test to match what it verifies.- REMOVE with an empty
Collection(LangChain4jEmbeddingStoreProducer.remove): an empty-collection body hitsstore.removeAll(ids)directly, which throws langchain4j's own opaque "cannot be null or empty" — the exact failure the null/empty guard was added to prevent. Consider treating an empty collection the same as a no-body REMOVE so it produces the clearIllegalArgumentException. - Empty
Listbody (LangChain4jEmbeddingsProducer.process): anyListroutes toprocessBatch→model.embedAll(emptyList)when empty; behavior is model-dependent. A guard or a quick test would make the empty-batch case explicit.
✅ Looks good
- Single-path token/finish-reason headers correctly mirrored in
populateBatchHeaders. - Text-segment preservation via header + body fallback in
addBatchis correct for the chained embed→store pipeline. - Generated catalog/endpoint-dsl files regenerated and committed.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
- Change @SInCE 4.22 to @SInCE 4.23 on CAMEL_LANGCHAIN4J_EMBEDDINGS and CAMEL_LANGCHAIN4J_TEXT_SEGMENTS constants (4.22.0 is already released, main is 4.23.0-SNAPSHOT) - Move langchain4j-embeddings and langchain4j-embeddingstore upgrade guide entries from camel-4x-upgrade-guide-4_22.adoc to camel-4x-upgrade-guide-4_23.adoc
- batchEmbeddingTokenUsage: assert INPUT_TOKEN_COUNT and TOTAL_TOKEN_COUNT headers are set and positive - REMOVE with empty Collection: throw IllegalArgumentException instead of passing through to store.removeAll(emptyCollection) - Batch embedding with empty List: throw IllegalArgumentException instead of passing through to model.embedAll(emptyList)
4 participants
Summary
Add batch
embedAll,addAll,removeAllsupport for the LangChain4j embedding components, enabling efficient RAG ingestion pipelines.Embeddings producer (
camel-langchain4j-embeddings)Listbody and route toprocessBatchwithmodel.embedAll()TEXT_SEGMENTSheader for seamless downstream chaining with the embedding storemain)Embedding store producer (
camel-langchain4j-embeddingstore)EMBEDDINGSheader withaddAllvariants (embeddings-only, with text segments, with caller IDs)EMBEDDING_ID(single) andEMBEDDING_IDS(batch) headersaddAllwith singleton lists (no text segment loss)callerIds,embeddings, andtextSegments— throwsIllegalArgumentExceptionon mismatchCollection<String>body orFilterheaderIllegalArgumentExceptionwith clear message (safe failure, no destructiveremoveAll()fallback)resolveEmbedding()/ auto-embedding pattern frommainShared constants (
core/camel-api)CAMEL_LANGCHAIN4J_EMBEDDINGSandCAMEL_LANGCHAIN4J_TEXT_SEGMENTStoCamelLangchain4jAttributesDocumentation & upgrade guide
Tests
LangChain4jEmbeddingsBatchTest— batch embedding with string list, token usage, single-item path verificationLangChain4jEmbeddingStoreBatchOperationsTest— 12 tests covering all add/remove variants, size mismatch validation, caller ID with text segment preservationReview comments addressed
All feedback from prior reviews has been incorporated in this rebased commit:
removeAll()fallback → throwsIllegalArgumentExceptionTEXT_SEGMENTSheaderadd(id, embedding)when no text segmentsaddAllwith singleton listsClaude Code on behalf of @gnodet