feat(rag): parent chunk expansion in query pipeline (FEAT-017)#88
Conversation
dataset-full.jsonl retargets the 55 questions to the eval-full namespace (clean full Italian Constitution from Wikisource, 72 chunks + official OHCHR UDHR PDF, 6 chunks). The excerpt corpus saturates hit rate at 100%; the full corpus is discriminative (hit 89.1%, MRR 0.806 at baseline). README documents corpus provenance, the senato.it PDF extraction trap, and how to run/extend the harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Store time: DualStrategyChunking parents now carry their own hierarchy id; run_ingest remaps chunker-local ids to deterministic stored ids (uuid5(doc_id, position)) and sets ChunkEmbedding.parent_id on children. Qdrant persists parent_id as a top-level payload key; pgvector fills the existing (always-NULL until now) parent_id column and honors caller ids. Search time: both providers exclude chunk_level=parent from search results (parents are context material, not retrieval targets). New VectorStoreProvider.retrieve(namespace, chunk_ids) fetches chunks by id. Query time: AdvancedQueryPipeline step 6.5 (VEKTRA_PARENT_EXPANSION_ENABLED, default false) replaces child text with the parent chunk text after the retrieval filter and before token budgeting (ARCH-055); children of the same parent collapse into the highest-scored one. Trace records children_expanded, siblings_merged, parents_fetched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…T-017) - ingest: dual ingest maps children to the parent's deterministic stored id (uuid5(doc_id, position)); parents and tables carry no parent_id - qdrant: parent_id in payload, must_not filter on chunk_level=parent, retrieve() namespace isolation and Record (no score) handling - pgvector: caller-provided uuid ids honored, parent_id column filled, dense search excludes parents, retrieve() maps rows and skips bad ids - advanced pipeline: expansion replaces child text with parent text, merges siblings, degrades gracefully on retrieve failure, keeps children when the parent is missing, and feeds the token budget with the expanded text; default-off leaves the pipeline untouched Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…T-025 fixture - plan: section 3 complete; Notes with measure A (dual, no expansion: retrieval -6.5pp hit from parent-boundary chunking, e2e unchanged) and measure B (expansion on: grounding-neutral, zero flips, sources 1.4->1.0 via sibling merge, expansion verified in traces) - backlog: FEAT-017 completed with premise corrections and honest numbers; TECH-007 filed with the multi-chunk collapse root cause (rerank scores for multi-part questions all below min_relevance_score 0.15; MC-01 max rerank 0.088 vs raw RRF 0.61 - the funnel wipes candidates before expansion can run) - changelog: FEAT-017 Added entry + Changed entries (parent exclusion in search, pgvector deterministic ids) - tests: DEBT-025 hermetic fixture replicated in vektra-core and vektra-ingest (ambient VEKTRA_CHUNKING_STRATEGY=dual from the dev .env broke 12 tests there; vektra-shared alone was covered) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughParent-child relationships are persisted during dual ingestion, supported by both vector stores, and optionally expanded in the advanced query pipeline. Configuration, tests, documentation, changelog entries, and evaluation data are added for the feature. ChangesParent Chunk Expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant QueryClient
participant AdvancedQueryPipeline
participant VectorStoreProvider
participant LLM
QueryClient->>AdvancedQueryPipeline: submit advanced query
AdvancedQueryPipeline->>VectorStoreProvider: search child chunks
VectorStoreProvider-->>AdvancedQueryPipeline: filtered child results
AdvancedQueryPipeline->>VectorStoreProvider: retrieve parent chunk IDs
VectorStoreProvider-->>AdvancedQueryPipeline: parent chunk text
AdvancedQueryPipeline->>LLM: construct prompt with expanded context
LLM-->>QueryClient: generated answer
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements parent chunk expansion (FEAT-017) in the advanced query pipeline, allowing retrieved child chunks to be replaced with their parent chunk's text before token budgeting. To support this, parent-child linkage is now persisted using deterministic UUIDs, parent chunks are excluded from default search results, and a new retrieve method is added to the vector store providers. The review feedback highlights a potential crash in Qdrant when retrieving invalid UUIDs, recommends optimizing parent ID deduplication from O(N^2) to O(N) using dict.fromkeys, and suggests updating logging in pgvector to use structlog with keyword-only arguments to align with the repository style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
….fromkeys Addresses review comments 3567194789 (invalid point ids would fail the whole retrieve batch server-side; now filtered and logged like pgvector) and 3567194791 (O(N^2) unique-preserving scan). +2 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vektra-ingest/src/vektra_ingest/chunking.py (1)
268-280: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring:
parent_id = Noneno longer accurate.The class docstring (lines 137-141) still states parent chunks have
parent_id = None, but this change now setsparent_idto the chunk's own hierarchy id. Please update the docstring to match the new FEAT-017 behavior described in the inline comment at Line 268.📝 Proposed docstring fix
Parent-child hierarchy (2 levels): - Level 0 (parent): every parent_chunk_size tokens, a parent chunk is - emitted with the full accumulated text. parent_id = None. + emitted with the full accumulated text. parent_id = its own + hierarchy id (FEAT-017), later remapped to a stored id by the + ingest pipeline. - Level 1 (child): normal fixed-size chunks with overlap. parent_id = ID of the enclosing parent chunk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-ingest/src/vektra_ingest/chunking.py` around lines 268 - 280, The class docstring incorrectly says parent chunks use parent_id = None. Update the docstring for the chunking class to state that parent chunks set parent_id to their own hierarchy ID, matching the FEAT-017 behavior implemented by the parent DocumentChunk construction and its inline comment.
🧹 Nitpick comments (3)
vektra-shared/src/vektra_shared/config.py (1)
501-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the Phase-2 flag out of
VektraSettings.Line 501 duplicates configuration already owned by
QueryPipelineConfig, violating the root model’s Phase-1-only boundary. Let the consuming advanced pipeline instantiateQueryPipelineConfigdirectly.Based on learnings,
VektraSettingsmust remain limited to Phase 1 variables while Phase 2 sub-configs validate independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-shared/src/vektra_shared/config.py` around lines 501 - 503, Remove parent_expansion_enabled from VektraSettings so the root model remains limited to Phase 1 configuration. Keep this setting owned and validated by QueryPipelineConfig, and update the advanced pipeline consumer to instantiate QueryPipelineConfig directly.Source: Learnings
vektra-index/tests/test_qdrant_provider.py (1)
282-297: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion only checks the exclusion key, not its match value.
condition_keyscollects thekeykwarg from everyFieldConditioncall (including themustconditions), so the test would still pass if themust_notcondition's match value were changed from"parent"to something else. Consider asserting on the specific call that has bothkey="metadata.chunk_level"andmatchbuilt withvalue="parent".As per path instructions, "Check that tests are genuinely testing behavior, not just mocking everything."
♻️ Proposed stronger assertion
filter_kwargs = _mock_qdrant_models.Filter.call_args.kwargs assert "must_not" in filter_kwargs assert len(filter_kwargs["must_not"]) == 1 - condition_keys = [ - c.kwargs.get("key") - for c in _mock_qdrant_models.FieldCondition.call_args_list - ] - assert "metadata.chunk_level" in condition_keys + chunk_level_calls = [ + c + for c in _mock_qdrant_models.FieldCondition.call_args_list + if c.kwargs.get("key") == "metadata.chunk_level" + ] + assert len(chunk_level_calls) == 1 + _mock_qdrant_models.MatchValue.assert_any_call(value="parent")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-index/tests/test_qdrant_provider.py` around lines 282 - 297, Strengthen test_build_filter_excludes_parent_chunks by inspecting the FieldCondition call corresponding to the must_not filter and asserting it uses key "metadata.chunk_level" with a match value of "parent". Do not rely on condition_keys alone, since it includes unrelated must conditions and does not verify the excluded value.Source: Path instructions
vektra-index/src/vektra_index/providers/pgvector.py (1)
338-352: 🚀 Performance & Scalability | 🔵 TrivialConsider indexing the
chunk_levelJSONB predicate.This filter now runs on every dense/sparse search call. As
document_chunksgrows, an unindexedchunk_metadata->>'chunk_level'extraction on every query could become a scan bottleneck. Consider a functional/expression index (e.g.,CREATE INDEX ON document_chunks ((chunk_metadata->>'chunk_level'))) or, longer-term, promotingchunk_levelto a dedicated indexed column.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-index/src/vektra_index/providers/pgvector.py` around lines 338 - 352, Add an index supporting the JSONB chunk_level extraction used by _exclude_parent_chunks, preferably a functional index on document_chunks for chunk_metadata->>'chunk_level', and ensure it is created through the project’s existing schema or migration mechanism. Keep the current filtering behavior unchanged, including rows without chunk_level.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vektra-core/src/vektra_core/advanced_pipeline.py`:
- Around line 414-419: Move the _expand_parents call to immediately follow the
retrieval filter, before the existing post_retrieval safeguard. Run the
SafeguardHook/post-retrieval processing on the expanded results before prompt
construction, while preserving the original child IDs as the filter identifiers
used by that safeguard.
In `@vektra-index/tests/test_pgvector_unit.py`:
- Around line 179-239: Update
test_store_honors_deterministic_ids_and_parent_linkage and
test_store_falls_back_to_random_id_on_non_uuid to stop patching
DocumentChunkOrm. Intercept objects passed to session.add, allow the real
SQLAlchemy ORM instances to be constructed, and assert their id and parent_id
attributes; retain the existing deterministic-ID, fallback-ID, and
parent-linkage expectations.
---
Outside diff comments:
In `@vektra-ingest/src/vektra_ingest/chunking.py`:
- Around line 268-280: The class docstring incorrectly says parent chunks use
parent_id = None. Update the docstring for the chunking class to state that
parent chunks set parent_id to their own hierarchy ID, matching the FEAT-017
behavior implemented by the parent DocumentChunk construction and its inline
comment.
---
Nitpick comments:
In `@vektra-index/src/vektra_index/providers/pgvector.py`:
- Around line 338-352: Add an index supporting the JSONB chunk_level extraction
used by _exclude_parent_chunks, preferably a functional index on document_chunks
for chunk_metadata->>'chunk_level', and ensure it is created through the
project’s existing schema or migration mechanism. Keep the current filtering
behavior unchanged, including rows without chunk_level.
In `@vektra-index/tests/test_qdrant_provider.py`:
- Around line 282-297: Strengthen test_build_filter_excludes_parent_chunks by
inspecting the FieldCondition call corresponding to the must_not filter and
asserting it uses key "metadata.chunk_level" with a match value of "parent". Do
not rely on condition_keys alone, since it includes unrelated must conditions
and does not verify the excluded value.
In `@vektra-shared/src/vektra_shared/config.py`:
- Around line 501-503: Remove parent_expansion_enabled from VektraSettings so
the root model remains limited to Phase 1 configuration. Keep this setting owned
and validated by QueryPipelineConfig, and update the advanced pipeline consumer
to instantiate QueryPipelineConfig directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5df3b9f-97cf-4939-ac49-4ea088f08fe4
⛔ Files ignored due to path filters (2)
.s2s/BACKLOG.mdis excluded by!.s2s/**.s2s/plans/20260712-sprint3-rag-quality.mdis excluded by!.s2s/**
📒 Files selected for processing (21)
.env.exampleCHANGELOG.mddocs/reference/configuration.mdtests/eval/README.mdtests/eval/dataset-full.jsonlvektra-core/src/vektra_core/advanced_pipeline.pyvektra-core/tests/conftest.pyvektra-core/tests/test_advanced_pipeline.pyvektra-index/src/vektra_index/adapters.pyvektra-index/src/vektra_index/providers/pgvector.pyvektra-index/src/vektra_index/providers/qdrant.pyvektra-index/tests/test_pgvector_unit.pyvektra-index/tests/test_qdrant_provider.pyvektra-ingest/src/vektra_ingest/chunking.pyvektra-ingest/src/vektra_ingest/pipeline.pyvektra-ingest/tests/conftest.pyvektra-ingest/tests/test_dual_chunking.pyvektra-ingest/tests/test_pipeline.pyvektra-shared/src/vektra_shared/config.pyvektra-shared/src/vektra_shared/protocols.pyvektra-shared/src/vektra_shared/types.py
| # Step 6.5: Parent chunk expansion (FEAT-017) | ||
| if self._parent_expansion and filtered: | ||
| t0 = time.monotonic() | ||
| filtered, expansion_meta = await self._expand_parents( | ||
| query.namespace, filtered | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Safeguard the expanded context before prompt construction.
Line 380 runs post_retrieval on child snippets, then this block injects uninspected parent text into the prompt. Move expansion immediately after the retrieval filter and run the post-retrieval safeguard on the expanded results, retaining child IDs as filter identifiers.
As per path instructions, vektra-core/** requires SafeguardHook integration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vektra-core/src/vektra_core/advanced_pipeline.py` around lines 414 - 419,
Move the _expand_parents call to immediately follow the retrieval filter, before
the existing post_retrieval safeguard. Run the SafeguardHook/post-retrieval
processing on the expanded results before prompt construction, while preserving
the original child IDs as the filter identifiers used by that safeguard.
Source: Path instructions
| async def test_store_honors_deterministic_ids_and_parent_linkage(self): | ||
| """store() keeps caller-provided uuid ids and fills the parent_id column.""" | ||
| from vektra_index.providers.pgvector import PgvectorProvider | ||
|
|
||
| session = self._make_session() | ||
| provider = PgvectorProvider() | ||
|
|
||
| parent_uuid = uuid4() | ||
| child_uuid = uuid4() | ||
| chunks = [ | ||
| ChunkEmbedding( | ||
| chunk_id=str(parent_uuid), | ||
| text="parent", | ||
| dense=[0.1] * 384, | ||
| metadata={"chunk_level": "parent"}, | ||
| ), | ||
| ChunkEmbedding( | ||
| chunk_id=str(child_uuid), | ||
| text="child", | ||
| dense=[0.1] * 384, | ||
| metadata={"chunk_level": "child"}, | ||
| parent_id=str(parent_uuid), | ||
| ), | ||
| ] | ||
|
|
||
| with patch("vektra_index.models.DocumentChunkOrm") as MockOrm: | ||
| MockOrm.return_value = MagicMock() | ||
| result = await provider.store(session, "default", uuid4(), chunks) | ||
|
|
||
| assert result == [str(parent_uuid), str(child_uuid)] | ||
| orm_kwargs = [c.kwargs for c in MockOrm.call_args_list] | ||
| assert orm_kwargs[0]["id"] == parent_uuid | ||
| assert orm_kwargs[0]["parent_id"] is None | ||
| assert orm_kwargs[1]["id"] == child_uuid | ||
| assert orm_kwargs[1]["parent_id"] == parent_uuid | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_store_falls_back_to_random_id_on_non_uuid(self): | ||
| """Non-UUID caller ids (external store-chunks callers) get random uuids.""" | ||
| from vektra_index.providers.pgvector import PgvectorProvider | ||
|
|
||
| session = self._make_session() | ||
| provider = PgvectorProvider() | ||
|
|
||
| chunks = [ | ||
| ChunkEmbedding( | ||
| chunk_id="not-a-uuid", | ||
| text="x", | ||
| dense=[0.1] * 384, | ||
| metadata={}, | ||
| parent_id="also-not-a-uuid", | ||
| ), | ||
| ] | ||
|
|
||
| with patch("vektra_index.models.DocumentChunkOrm") as MockOrm: | ||
| MockOrm.return_value = MagicMock() | ||
| result = await provider.store(session, "default", uuid4(), chunks) | ||
|
|
||
| UUID(result[0]) # random but valid | ||
| assert MockOrm.call_args.kwargs["parent_id"] is None | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Don't mock the SQLAlchemy ORM class — use the session.add intercept pattern.
Both test_store_honors_deterministic_ids_and_parent_linkage and test_store_falls_back_to_random_id_on_non_uuid patch DocumentChunkOrm itself with a MagicMock, then assert on the mock's constructor kwargs. This only verifies what arguments were passed to the (fake) constructor, not that a real DocumentChunkOrm instance was correctly built. As per path instructions, "Prefer real objects over mocks. Check that SQLAlchemy ORM classes are never mocked (use session.add intercept pattern instead)."
♻️ Proposed fix using the intercept pattern
- with patch("vektra_index.models.DocumentChunkOrm") as MockOrm:
- MockOrm.return_value = MagicMock()
- result = await provider.store(session, "default", uuid4(), chunks)
-
- assert result == [str(parent_uuid), str(child_uuid)]
- orm_kwargs = [c.kwargs for c in MockOrm.call_args_list]
- assert orm_kwargs[0]["id"] == parent_uuid
- assert orm_kwargs[0]["parent_id"] is None
- assert orm_kwargs[1]["id"] == child_uuid
- assert orm_kwargs[1]["parent_id"] == parent_uuid
+ added: list = []
+ session.add = lambda obj: added.append(obj)
+
+ result = await provider.store(session, "default", uuid4(), chunks)
+
+ assert result == [str(parent_uuid), str(child_uuid)]
+ assert added[0].id == parent_uuid
+ assert added[0].parent_id is None
+ assert added[1].id == child_uuid
+ assert added[1].parent_id == parent_uuidThe same fix applies to test_store_falls_back_to_random_id_on_non_uuid.
As per path instructions for tests/**: "Check that SQLAlchemy ORM classes are never mocked (use session.add intercept pattern instead)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vektra-index/tests/test_pgvector_unit.py` around lines 179 - 239, Update
test_store_honors_deterministic_ids_and_parent_linkage and
test_store_falls_back_to_random_id_on_non_uuid to stop patching
DocumentChunkOrm. Intercept objects passed to session.add, allow the real
SQLAlchemy ORM instances to be constructed, and assert their id and parent_id
attributes; retain the existing deterministic-ID, fallback-ID, and
parent-linkage expectations.
Source: Path instructions
1 participant
What
uuid5(doc_id, position)ids,ChunkEmbedding.parent_id,parent_idin the Qdrant payload and in the (previously always-NULL) pgvector columnchunk_level=parentin both providers (Qdrantmust_not, pgvectorIS DISTINCT FROM); parents are context material, not retrieval targetsVectorStoreProvider.retrieve(namespace, chunk_ids)Protocol method (both providers + session-managed adapter)parent_expansion(VEKTRA_PARENT_EXPANSION_ENABLED, default off): replaces child text with the parent chunk text after the retrieval filter and before token budgeting (ARCH-055); children of the same parent collapse into the highest-scored one; trace recordschildren_expanded/siblings_merged/parents_fetchedstore()honors caller-provided UUID chunk ids (falls back to random for non-UUID ids)vektra-coreandvektra-ingest(ambientVEKTRA_CHUNKING_STRATEGY=dualfrom a dev.envbroke 12 tests there)tests/eval/dataset-full.jsonl, nseval-full) + harness READMEWhy
parent_idanywhere, so the plumbing is most of this PR.Measured (eval-full corpus, Combo D, details in plan Notes)
Details
Plan:
.s2s/plans/20260712-sprint3-rag-quality.mdsection 3 (all items checked)Checklist
make lintgreen)🤖 Generated with Claude Code
Summary by CodeRabbit