docs: dn_redis is a key-shape protocol + Rust command type model#455
Conversation
Clarifies that the dn_redis module in lance-graph-cognitive is a PROTOCOL/SHAPE adapter, not a service dependency. Three valid backends: external Redis (a); in-binary OGIT class view + DataFusion query over Lance (b, the zero-boundary target); anything that produces the documented ada:dn key shape (c). Same pattern as FalkorDB (formerly RedisGraph) and KuzuDB — talk Redis without BEING Redis. Adopters reading the dn_redis module + the lab-vs-canonical discipline docs sometimes conclude that an external Redis server is required; this doc clarifies that 'protocol-only adapter' means the SHAPE is the contract, not a service binding. Doc-only; new file at docs/DN_REDIS_EMULATION.md. Pairs naturally with the cluster-asymmetry + append-only-Raft docs (PR #452/#453/ #454) — all three address common mis-reads of lance-graph idioms through a legacy lens. Provenance: - Surfaced by: bardioc PR #15 conversation thread - Draft outline: bardioc/UPSTREAM_LANCE_GRAPH_PR_CANDIDATES.md T1.5 - Sister doc: APPEND_ONLY_RAFT_DOVETAIL.md (PR #452/#454) - Sister doc: CLUSTER_ASYMMETRY.md (PR #453/#454)
📝 WalkthroughWalkthroughThis PR updates the ChangesRedis Protocol Adapter Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aac812162f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| This doc clarifies that **"protocol-only adapter" means the SHAPE is the contract — the backend is open**. Three valid backends: | ||
|
|
||
| - (a) an external Redis cluster (traditional hybrid deployment) |
There was a problem hiding this comment.
For consumers selecting option (a) with an actual Redis Cluster, the documented walk_to_root/mget_dns operations will generally fail because dn_key produces keys such as ada:dn:0102... without a shared Redis hash tag, while Redis Cluster supports multi-key operations only when every key belongs to the same hash slot (Redis Cluster specification). The adapter's MGET commands therefore make an external Redis cluster an invalid backend unless the key layout is redesigned or the backend fans the reads out, so this should not be listed as unconditionally valid.
Useful? React with 👍 / 👎.
|
|
||
| [FalkorDB](https://www.falkordb.com/) (formerly RedisGraph) and [KùzuDB](https://kuzudb.com/) both "talk Redis" — they expose the Redis wire protocol — while the storage and execution underneath are completely different. They are NOT Redis; they emulate the wire protocol so existing Redis clients work. | ||
|
|
||
| `dn_redis` follows the same pattern. It defines: |
There was a problem hiding this comment.
The dn_redis module does not follow the stated wire-protocol-emulation pattern: it only constructs Rust String keys and RedisCommand enum values, and contains no RESP encoder/parser, listener, command executor, or backend interface. As a result, existing Redis clients cannot communicate with it and an in-binary DataFusion implementation would not emit Redis wire-protocol responses without a separate new layer; describing the module as wire-protocol emulation will mislead consumers about what is already implemented.
Useful? React with 👍 / 👎.
|
|
||
| For the in-binary backend (option b): | ||
|
|
||
| 1. Implement a `dn_redis::Backend` impl (or whatever the trait name resolves to in the adapter's surface) that intercepts the documented operations |
There was a problem hiding this comment.
For a consumer attempting the recommended in-binary backend, this first step cannot be followed: dn_redis exports no Backend trait or equivalent execution surface, and the repository has no call sites that mount either a Redis client or a backend for RedisPipeline. The parenthetical placeholder does not identify a resolvable API, so the document should either specify the new interface consumers must create themselves or avoid presenting this as an implementation procedure.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/DN_REDIS_EMULATION.md (1)
15-15: ⚡ Quick winAvoid hardcoding module line count in docs.
“253-line protocol-only adapter” will drift quickly and create doc staleness; prefer a timeless description without exact LOC.
🤖 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 `@docs/DN_REDIS_EMULATION.md` at line 15, Remove the hardcoded "253-line" reference and any other exact LOC mentions from the DN_REDIS_EMULATION.md description of lance-graph-cognitive::container_bs::dn_redis; instead describe it as a "protocol-only adapter" (or "small/lightweight protocol-only adapter") so the text remains timeless, and update any SPRINT plan docs that reference dn_redis as "253-line protocol-only" to the new phrasing to avoid future staleness.
🤖 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 `@docs/DN_REDIS_EMULATION.md`:
- Line 42: The doc currently uses speculative wording "dn_redis::Backend (or
whatever the trait name resolves to)"; replace that with the actual exported
symbol from the dn_redis crate (e.g., the exact trait/type name exported for
adapter backends) or explicitly mark the snippet as pseudocode. Update the
sentence to either reference the verified exported symbol (for example:
dn_redis::BackendTrait — use the real exported name you find in the crate) or
change phrasing to "Implement a backend that implements the following pseudocode
trait:" and keep the remainder as pseudocode; ensure you update any further
references in the document to use the same concrete symbol or the explicit
pseudocode label (search for "Backend" and similar occurrences to keep
consistency).
---
Nitpick comments:
In `@docs/DN_REDIS_EMULATION.md`:
- Line 15: Remove the hardcoded "253-line" reference and any other exact LOC
mentions from the DN_REDIS_EMULATION.md description of
lance-graph-cognitive::container_bs::dn_redis; instead describe it as a
"protocol-only adapter" (or "small/lightweight protocol-only adapter") so the
text remains timeless, and update any SPRINT plan docs that reference dn_redis
as "253-line protocol-only" to the new phrasing to avoid future staleness.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 789c3ff8-c942-4887-986b-b66dd63649fd
📒 Files selected for processing (1)
docs/DN_REDIS_EMULATION.md
…tocol emulation Four review findings on PR #455 walked back, all real: Codex P2 1 - Redis Cluster is not plug-and-play. The walk_to_root / mget_dns operations use MGET, but the current key layout (ada:dn:{hex}) has no shared hash tag. Redis Cluster requires multi-key ops to belong to the same hash slot. So external Redis CLUSTER is NOT a valid backend without re-keying with hash tags. Added explicit caveat. Codex P2 2 - dn_redis is NOT wire-protocol emulation. The module exports Rust String keys + a RedisCommand enum + RedisPipeline. There is no RESP encoder, no parser, no listener, no command executor. Existing Redis clients cannot communicate with it. The FalkorDB / KuzuDB precedent does NOT apply (those projects implement the actual RESP wire protocol; dn_redis does not). Reframed the doc to "key-shape protocol + Rust command type model" - which is what the module actually is. Codex P2 3 - There is no dn_redis::Backend trait to implement. The "Recommended consumer pattern" first step cited a nonexistent API. Rewrote the section to show what consumer code actually has to provide: a struct + execute function that takes RedisCommand and runs it against the chosen backend. The example is illustrative (not a shipped trait). CodeRabbit minor - Same critique class. The speculative API wording is gone. Net effect: the doc now accurately describes what dn_redis IS (key-shape + command type model) and what adopters have to provide (an executor). Two valid backend categories enumerated honestly: (A) standalone Redis with optional hash-tag re-keying for clustered deployments; (B) in-binary executor over Lance via DataFusion. File renamed: DN_REDIS_EMULATION.md -> DN_REDIS_KEY_SHAPE_PROTOCOL.md to match the corrected framing. Provenance: codex P2 + coderabbit minor on PR #455 commit aac8121.
AdaWorldAPI
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/DN_REDIS_KEY_SHAPE_PROTOCOL.md`:
- Around line 50-68: The example shows an async call to
self.lance.read_by_dn(...).await inside LanceBackend::execute but execute is
declared synchronously; change the signature of execute to async (async fn
execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error>) so await is
valid, and ensure any call sites handle the returned Future, or alternatively
remove .await and make read_by_dn synchronous; update the LanceBackend::execute
and any related usages accordingly to keep consistency between execute and
read_by_dn.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c55d01b-50af-43e0-8b60-32406ed14962
📒 Files selected for processing (1)
docs/DN_REDIS_KEY_SHAPE_PROTOCOL.md
| ```rust | ||
| // Consumer code, NOT lance-graph code | ||
| struct LanceBackend { /* ... */ } | ||
|
|
||
| impl LanceBackend { | ||
| fn execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error> { | ||
| match cmd { | ||
| RedisCommand::Get(key) => { | ||
| let dn = self.parse_dn_from_key(&key)?; | ||
| let row = self.lance.read_by_dn(dn).await?; | ||
| Ok(RedisValue::Bulk(cog_record_to_bytes(&row))) | ||
| } | ||
| RedisCommand::Mget(keys) => { /* batch lookup over Lance */ } | ||
| RedisCommand::Keys(pattern) => { /* DataFusion scan filtered by prefix */ } | ||
| // ... etc per the enum's variants | ||
| } | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix async/await syntax error in code example.
Line 59 uses .await but the execute function on line 55 is not declared as async. This will not compile.
Either declare the function as async fn:
async fn execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error>Or remove the .await and make the hypothetical read_by_dn method synchronous. Since this is an example showing the pattern consumers should implement, the async version is likely more realistic for I/O operations.
🔧 Proposed fix
impl LanceBackend {
- fn execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error> {
+ async fn execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error> {
match cmd {
RedisCommand::Get(key) => {🤖 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 `@docs/DN_REDIS_KEY_SHAPE_PROTOCOL.md` around lines 50 - 68, The example shows
an async call to self.lance.read_by_dn(...).await inside LanceBackend::execute
but execute is declared synchronously; change the signature of execute to async
(async fn execute(&self, cmd: RedisCommand) -> Result<RedisValue, Error>) so
await is valid, and ensure any call sites handle the returned Future, or
alternatively remove .await and make read_by_dn synchronous; update the
LanceBackend::execute and any related usages accordingly to keep consistency
between execute and read_by_dn.
1 participant
Summary
Clarifies that
lance-graph-cognitive::container_bs::dn_redisis a Redis wire-protocol EMULATION, NOT a service dependency on Redis. Adopters reading the module + the surrounding lab-vs-canonical discipline docs sometimes conclude an external Redis server is required; this doc names the FalkorDB/KùzuDB precedent and lists three valid backends.The three valid backends
dn_redisadapter shape — the zero-boundary single-binary targetada:dn:{hex}/ada:spine:{hex}key shape with the documented operationsWhy this matters
For single-binary deployments (e.g.
AdaWorldAPI/bardioc/substrate-b) the natural backend is (b): the in-binary view. This means the "zero application-level boundaries" deployment shape (per the PR #452/#454 append-only-Raft doc) survives the addition of HHTL-keyed hot lookups precisely becausedn_redisis EMULATION, not a service.The pattern is well-established outside lance-graph: FalkorDB (formerly RedisGraph) and KùzuDB both "talk Redis" while their storage + execution layers are completely different.
dn_redisborrows the wire-protocol-emulation pattern.Provenance
AdaWorldAPI/bardiocPR docs: add hot/cold path architecture and documentation drift audit #15 conversation threadbardioc/UPSTREAM_LANCE_GRAPH_PR_CANDIDATES.mdT1.5APPEND_ONLY_RAFT_DOVETAIL.md(PR docs: Lance append-only model dovetails with Raft replication #452 merged, fixes via PR docs: post-merge corrections on PR #452 + #453 architecture docs #454) andCLUSTER_ASYMMETRY.md(PR docs: cluster asymmetry — capacity-forced vs availability-chosen clustering #453 merged, fixes via PR docs: post-merge corrections on PR #452 + #453 architecture docs #454)Asks
docs/DN_REDIS_EMULATION.md; alternative:.claude/knowledge/dn-redis-emulation.mdif AdaWorldAPI prefers the Claude-Code knowledge layer)Doc-only; zero code; pairs with PR #454.
Summary by CodeRabbit