[FEATURE] Add pluggable ChunkStore interface for chunk text storage - #436
Draft
noel-improv wants to merge 3 commits into
Draft
[FEATURE] Add pluggable ChunkStore interface for chunk text storage#436noel-improv wants to merge 3 commits into
noel-improv wants to merge 3 commits into
Conversation
Chunk text lives inline as a chunk.value graph property today, which Neptune keeps in memory for every node whether or not a query reads it. This adds a ChunkStore interface (get/put/get_batch) with a ChunkStoreFactory registration pattern mirroring GraphStoreFactory, and an InGraphChunkStore implementing today's behavior as the default. No existing write or read path is wired to it yet — this is the interface and default backend only, laying the groundwork for an external (e.g. S3) backend in a follow-up.
Every backend implementing ChunkStore ends up writing the same get_batch([chunk_id]).get(chunk_id) one-liner for get(). Make get() a concrete method on the ABC instead of forcing each implementation to repeat it; only put()/get_batch() stay abstract.
…reFactory InGraphChunkStoreFactory checked isinstance(graph_store, GraphStore), but GraphBatchClient - what real graph builders actually pass as graph_client - duck-types GraphStore without subclassing it, matching how the rest of the codebase already treats this parameter. The isinstance check would have rejected it once ChunkGraphBuilder wires up to ChunkStore. Check for None instead of type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Chunk text is stored inline as a
chunk.valueproperty on every__Chunk__graph node. Neptune keeps all node properties in memory regardless of whether a query reads them, so at scale this is a large, avoidable memory cost. This PR adds aChunkStoreinterface for chunk text (get/put/get_batch) with a factory registration pattern, and anInGraphChunkStoreimplementation that preserves today's behavior as the default. This PR is interface-only: no existing write or read path is wired to the new interface yet, so there is no behavior change. WiringChunkGraphBuilderand the read paths toChunkStore, and adding an external (e.g. S3) backend, are follow-up PRs that depend on this one and should merge after it.Changes
ChunkStore, an abstract interface for chunk text storage.put/get_batchare abstract;gethas a default implementation (get_batch([chunk_id]).get(chunk_id)) so backends only need to implementget_batchto get single-chunk lookups for free.InGraphChunkStore, which reads and writeschunk.valueon__Chunk__nodes, matching the existence semantics of the currentget_chunks_queryread path (a chunk without a valid__EXTRACTED_FROM__link to a__Source__is excluded).ChunkStoreFactory, a registration pattern mirroringGraphStoreFactory.InGraphChunkStoreFactoryis registered as an explicit last-resort fallback, tried only after any other registered factory, regardless of registration order.graph_utils.to_params()helper for theUNWIND $params-style Cypher parameter wrapping already used byGraphBuilder._to_params;InGraphChunkStore.put()reuses it instead of duplicating the shape inline.Problem
Related issue (if any): #324
Testing
pytest)New unit tests cover the
ChunkStoreabstract contract,InGraphChunkStore's get/get_batch/put behavior against a mocked graph client, andChunkStoreFactory's registration and fallback-ordering behavior. I also ran the new code against a real Neo4j instance (via a local container) to confirm the Cypher actually executes as written, since mocked unit tests can't catch a malformed query — this caught and fixed a real bug (a missing parameter-wrapping step) before it reached review.Checklist
No new backend is wired into any existing write or read path, so there is nothing to migrate and no behavior change for current users.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.