feat: add KGSearchRetrieval for full KG pipeline (N-hop, scoring, query_rewrite, community)#15690
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds KG data types, chunk parsers, hybrid search expression builders, an LLM-backed query-rewrite, a configurable KGSearchPipeline orchestration, CSV/score formatting updates, expanded unit tests, and tenant nil-DAO guards. ChangesKnowledge Graph Retrieval & Scoring
Sequence DiagramsequenceDiagram
participant Client
participant KGSearchPipeline
participant LLM as QueryRewriteLLM
participant DocEngine
participant Scoring as KGScoring
participant Community as CommunitySearch
Client->>KGSearchPipeline: Retrieval(question, kbIDs, tenantIDs)
KGSearchPipeline->>LLM: queryRewrite(question, type samples)
LLM-->>KGSearchPipeline: typeKeywords, entities
KGSearchPipeline->>DocEngine: Search(entities/types) concurrently
DocEngine-->>KGSearchPipeline: entity chunks, relation chunks, type samples
KGSearchPipeline->>Scoring: N-hop analysis & score fusion
Scoring-->>KGSearchPipeline: scored entities & relations
KGSearchPipeline->>Community: searchKGCommunityContent(entities, remainingTokens)
Community-->>KGSearchPipeline: community report chunks
KGSearchPipeline-->>Client: composed result map (content_with_weight, metadata)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
dosubot
Bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/service/kg_retrieval_test.go (1)
36-38: 💤 Low valueUnused mock code: entity_kwd filter branch never matches.
The mock checks for
entity_kwdinreq.Filterto build composite keys, but the realKGSearchRetrievalimplementation places entities inMatchExprsrather than theFiltermap (see context snippet 1, lines 95-102). None of the test cases use this branch, so it's effectively dead code.♻️ Simplify the mock by removing the unused branch
func (m *mockRetrievalEngine) Search(ctx context.Context, req *types.SearchRequest) (*types.SearchResult, error) { kgType, _ := req.Filter["knowledge_graph_kwd"].(string) - key := kgType - if ents, ok := req.Filter["entity_kwd"].([]interface{}); ok && len(ents) > 0 { - key = kgType + ":" + ents[0].(string) - } + key := kgType if r, ok := m.results[key]; ok { return r, nil }🤖 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 `@internal/service/kg_retrieval_test.go` around lines 36 - 38, Remove the dead branch that checks req.Filter["entity_kwd"] in the mock used by the tests: the real KGSearchRetrieval places entities in MatchExprs, so the mock's entity_kwd branch never matches; update the mock (in kg_retrieval_test.go) to build composite keys solely from req.MatchExprs (or remove the entity_kwd check and unused variable key assembly) and simplify the mocked behavior in the function that currently inspects req.Filter to reflect the real implementation's use of MatchExprs.internal/service/kg_retrieval.go (2)
136-144: 💤 Low valueConsider logging errors from supplementary searches.
Type search and relation search errors are silently ignored for graceful degradation, but this could make debugging difficult. Consider logging these errors at debug/warn level.
Example for type search
typesResult, err := docEngine.Search(ctx, typesReq) + if err != nil { + // Log at debug/warn level - continue with empty type results + } entsFromTypes := make(map[string]struct{}) if err == nil {🤖 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 `@internal/service/kg_retrieval.go` around lines 136 - 144, The code currently swallows errors from the supplementary type search (the call to docEngine.Search with typesReq) making debugging hard; modify the block around typesResult, err := docEngine.Search(ctx, typesReq) to log the error (using the package logger or processLogger/ambient logger used elsewhere) at debug or warn level when err != nil, but keep the existing graceful-degradation behavior (i.e., do not return the error). Ensure the log includes context like the operation name ("type search") and the request details (typesReq or a minimal identifying field) and reference the entsFromTypes population logic so the flow remains unchanged when the search fails.
31-31: 💤 Low valueUnused
nameparameter.The
nameparameter is never used within the function body. At the call site (line 118), the entity name is extracted fromchunk["entity_kwd"]and used as the map key. Consider removing this parameter to clarify the API.♻️ Proposed fix
-func kgEntityFromChunk(name string, chunk map[string]interface{}) common.KGEntity { +func kgEntityFromChunk(chunk map[string]interface{}) common.KGEntity {And update the call site at line 118:
- e := kgEntityFromChunk(name, chunk) + e := kgEntityFromChunk(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 `@internal/service/kg_retrieval.go` at line 31, Remove the unused name parameter from kgEntityFromChunk and update its signature to func kgEntityFromChunk(chunk map[string]interface{}) common.KGEntity; then update all call sites that currently pass a name (notably the call that extracts entity from chunk["entity_kwd"]) to pass only the chunk. Ensure any references inside kgEntityFromChunk that expected name instead use chunk["entity_kwd"] (or the existing extraction logic) and adjust unit tests or callers accordingly.
🤖 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 `@internal/service/kg_retrieval.go`:
- Line 184: Update the token limit variable maxToken from 8196 to the intended
8192 (the common 2^13 token boundary) in internal/service/kg_retrieval.go; if
8196 was intentional, instead add a brief inline comment next to maxToken
explaining the reason for the non-standard value so readers aren’t confused.
Ensure you modify the declaration of maxToken and include the explanatory
comment if you keep the 8196 value.
- Around line 99-106: KGSearchRetrieval currently only uses entities[0] for both
entity and relation matching; change entsReq.MatchExprs and relsReq.MatchExprs
to not ignore additional rewritten entities by replacing MatchingText:
entities[0] with a joined string of all entities (e.g., strings.Join(entities,
", ")) for the entity search (update where entsReq.MatchExprs is assembled) and
align the relation search behavior with Python by using the original raw
question text (the incoming query variable) for relation MatchingText in
relsReq.MatchExprs (or, if you prefer parity via entities, also use the joined
entities there); update the code in KGSearchRetrieval where entsReq.MatchExprs
and relsReq.MatchExprs are constructed to use these new values.
---
Nitpick comments:
In `@internal/service/kg_retrieval_test.go`:
- Around line 36-38: Remove the dead branch that checks req.Filter["entity_kwd"]
in the mock used by the tests: the real KGSearchRetrieval places entities in
MatchExprs, so the mock's entity_kwd branch never matches; update the mock (in
kg_retrieval_test.go) to build composite keys solely from req.MatchExprs (or
remove the entity_kwd check and unused variable key assembly) and simplify the
mocked behavior in the function that currently inspects req.Filter to reflect
the real implementation's use of MatchExprs.
In `@internal/service/kg_retrieval.go`:
- Around line 136-144: The code currently swallows errors from the supplementary
type search (the call to docEngine.Search with typesReq) making debugging hard;
modify the block around typesResult, err := docEngine.Search(ctx, typesReq) to
log the error (using the package logger or processLogger/ambient logger used
elsewhere) at debug or warn level when err != nil, but keep the existing
graceful-degradation behavior (i.e., do not return the error). Ensure the log
includes context like the operation name ("type search") and the request details
(typesReq or a minimal identifying field) and reference the entsFromTypes
population logic so the flow remains unchanged when the search fails.
- Line 31: Remove the unused name parameter from kgEntityFromChunk and update
its signature to func kgEntityFromChunk(chunk map[string]interface{})
common.KGEntity; then update all call sites that currently pass a name (notably
the call that extracts entity from chunk["entity_kwd"]) to pass only the chunk.
Ensure any references inside kgEntityFromChunk that expected name instead use
chunk["entity_kwd"] (or the existing extraction logic) and adjust unit tests or
callers accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bebc9d9-dee3-4c0b-bf51-8a8e017947d1
📒 Files selected for processing (2)
internal/service/kg_retrieval.gointernal/service/kg_retrieval_test.go
| scoredRels := common.SortAndTrimRelations(relsFromText, 6) | ||
|
|
||
| // 9. Build KG content (entities + relations) with token budget, matching Python order | ||
| maxToken := 8196 |
Contributor
There was a problem hiding this comment.
Likely typo: 8196 should be 8192.
The token limit 8196 appears to be a typo for 8192 (which is 2^13, a common token limit). If this is intentional, consider adding a comment explaining the choice.
🔧 Proposed fix
- maxToken := 8196
+ maxToken := 8192📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| maxToken := 8196 | |
| maxToken := 8192 |
🤖 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 `@internal/service/kg_retrieval.go` at line 184, Update the token limit
variable maxToken from 8196 to the intended 8192 (the common 2^13 token
boundary) in internal/service/kg_retrieval.go; if 8196 was intentional, instead
add a brief inline comment next to maxToken explaining the reason for the
non-standard value so readers aren’t confused. Ensure you modify the declaration
of maxToken and include the explanatory comment if you keep the 8196 value.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/kg_retrieval.go (1)
138-146: 💤 Low valueConsider logging search errors for types and relations instead of silently ignoring.
Errors from
docEngine.Searchfor types (line 138) and relations (line 164) are silently swallowed. While the fallback to empty maps is safe for correctness, this could hide transient failures or misconfigurations in production.Adding debug-level logging would aid troubleshooting without changing behavior.
Also applies to: 164-174
🤖 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 `@internal/service/kg_retrieval.go` around lines 138 - 146, The docEngine.Search error for types (call site producing typesResult and entsFromTypes) and for relations (call site producing relationsResult and entsFromRelations) is currently ignored; update both search error branches to emit debug (or warn) logs including the error and contextual info (e.g., the request object or which search failed) using the existing logger before falling back to the empty map so behavior doesn't change—look for the calls to docEngine.Search and add a processLogger.Debug/Warn (or the package logger) that logs the error and a short message identifying "types search" or "relations search".
🤖 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.
Nitpick comments:
In `@internal/service/kg_retrieval.go`:
- Around line 138-146: The docEngine.Search error for types (call site producing
typesResult and entsFromTypes) and for relations (call site producing
relationsResult and entsFromRelations) is currently ignored; update both search
error branches to emit debug (or warn) logs including the error and contextual
info (e.g., the request object or which search failed) using the existing logger
before falling back to the empty map so behavior doesn't change—look for the
calls to docEngine.Search and add a processLogger.Debug/Warn (or the package
logger) that logs the error and a short message identifying "types search" or
"relations search".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f77c0bb-53e4-4857-8430-872fafe71df2
📒 Files selected for processing (2)
internal/service/kg_retrieval.gointernal/service/kg_retrieval_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/kg_retrieval_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15690 +/- ##
=======================================
Coverage 93.16% 93.16%
=======================================
Files 10 10
Lines 717 717
Branches 118 118
=======================================
Hits 668 668
Misses 29 29
Partials 20 20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
size:XL
This change eliminates the duplicate filtering implementation between service.ApplyMetaFilter and common.MetaFilter by removing the applySingleCondition adapter layer and consolidating all operator handling into common.MetaFilter. Key changes: - Remove applySingleCondition (65 lines of inline switch logic) - ApplyMetaFilter now converts MetaFilterCondition to MetaCondition and delegates directly to common.MetaFilter once per filter set - Add filterSet: O(n+m) hash-map fast path for in/not in operators (replaces O(n*m) linear scan in matchValue) - Export NormalizeOperator from common for operator alias mapping (includes ==, !=, >=, <=, is, not is aliases) Bug fixes: - contains/not contains: restored case-insensitive matching (ToLower) - not in: restored case-insensitive matching (EqualFold) - != with date filter: non-date metadata values now correctly match the != operator (they are not equal to the date) - Added == to operatorMapping so both ParseAndConvert and convertToMetaCondition normalize it to = Performance: - in/not in: O(n*m) linear scan -> O(n+m) hash-map lookup via filterSet - = operator delegates to common.MetaFilter (O(n) iteration for correctness/case-insensitivity, matching Python behavior) Cleanup: - Removed 18 lines of dead code (matchValue's in/not in branches bypassed by filterOut delegation) - Fixed orphaned godoc comment for convertOperator (reordered with NormalizeOperator) - Fixed filterSet doc comment: removed incorrect 'matching EqualFold' claim - Completed convertToMetaCondition operator normalization comment Tests: 60 tests (24 service + 36 common), all passing. New tests cover ==, !=, >=, <=, >, <, empty, not empty operators through ApplyMetaFilter; <, <=, !=, not-in-empty-list through MetaFilter/filterSet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file was accidentally included in the previous commit via git add internal/. The change was pre-existing in the working tree but unrelated to the metadata filter refactoring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When f.Value is "A,,B" or "A, ,B", strings.Split produces empty or whitespace-only entries. These are now filtered out after TrimSpace to prevent unintended matches against empty-string metadata values. Add 5 test cases: - InEmptyParts: A,,B produces [A B] - InOnlyWhitespaceParts: A, , ,B produces [A B] - InAllEmptyParts: ,,, produces [] - NotInEmptyParts: A,,B produces [A B] - InMixedSpaces: spaces around values preserve trimmed values Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Combines entity/relation/community search with N-hop analysis, score fusion, and token-budgeted content assembly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…community second)
- Add queryRewrite function that calls BuildQueryRewritePrompt + ChatWithMessages + ParseQueryRewriteResponse - Fall back to raw question when chatModel is nil (backward compatible) - Remove extractTypeKeywords and extractEntities stubs - Update tests for fallback and empty question scenarios Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Use strings.Join(entities, " ") so multiple entity names are all passed to the ES search query, matching Python's behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Extract KGSearchPipeline struct from monolithic KGSearchRetrieval - Parallelize three independent searches (entity/types/relations) with sync.WaitGroup - Move scoring/formatting/types from internal/common/ to internal/service/ - Fix nil ModelDriver panic in buildSearchExprs - Fix searchKGTypeSamples error discarded (add common.Warn) - Add FilterChunksByScore for post-search _score filtering (Python alignment) - Fix Embed input to use matchText.MatchingText instead of question - Add Python alignment constants (simThreshold=0.3, denseTopK=1024) - Fix community report format (JSON parse, Evidences section, numbering) - Add contract validation to mocks (IndexNames, KbIDs) - Convert fakeEmbedDriver stub to spyEmbedDriver with captured inputs - Fix relation test chunks missing _score - Fix bare type assertion in TestBuildSearchExprs_NoEmbModel - Add searchCaptureEngine IndexNames validation - Fix nil userTenantDAO panic in RemoveMember/AcceptInvite - Recover accidentally truncated task_test.go - 80+ tests covering all paths Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/service/kg_retrieval.go (1)
112-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify
maxToken: 8196— likely typo for 8192.A past review comment flagged this as a probable typo. 8192 (2^13) is a standard token limit, while 8196 is unusual. If 8196 is intentional, add a comment explaining the +4 offset; otherwise, correct to 8192.
🤖 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 `@internal/service/kg_retrieval.go` at line 112, The maxToken value set to 8196 on the KG retrieval configuration (symbol: maxToken) appears to be a typo; change it to 8192 (the standard 2^13 token limit) unless the +4 is intentional—if intentional, add an inline comment next to the maxToken assignment explaining why the extra 4 tokens are required so future readers understand the offset.internal/service/kg_pipeline.go (1)
101-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify
maxToken: 8196— likely typo for 8192.Same issue as in
kg_retrieval.go:112. If 8196 is intentional, document the rationale; otherwise, correct to 8192 (2^13).🤖 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 `@internal/service/kg_pipeline.go` at line 101, The maxToken value is set to 8196 (symbol: maxToken) which appears to be a typo—change it to 8192 (2^13) or, if 8196 was intentional, add a comment/docstring next to the maxToken assignment explaining the rationale; also mirror the same correction or documentation as done for kg_retrieval.go:112 so both places are consistent.internal/service/kg_types.go (1)
34-38:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: KGRelation redeclared — compilation blocked.
The pipeline reports
KGRelation redeclared in this block; other declaration in internal/service/kg_search.go:39:6. This is the same root cause as theKGEntityredeclaration above.🤖 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 `@internal/service/kg_types.go` around lines 34 - 38, KGRelation (and the earlier KGEntity) are defined twice which causes a redeclaration compile error; remove the duplicate type definition from this file and reuse the single canonical type instead (or merge any differing fields into one shared definition). Concretely: delete the KGRelation declaration in this diff (or reconcile its fields with the existing KGRelation/KGEntity declaration) so only one type named KGRelation exists; update any local references to use that single definition and ensure imports/builds compile against the unified type.
🤖 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.
Duplicate comments:
In `@internal/service/kg_pipeline.go`:
- Line 101: The maxToken value is set to 8196 (symbol: maxToken) which appears
to be a typo—change it to 8192 (2^13) or, if 8196 was intentional, add a
comment/docstring next to the maxToken assignment explaining the rationale; also
mirror the same correction or documentation as done for kg_retrieval.go:112 so
both places are consistent.
In `@internal/service/kg_retrieval.go`:
- Line 112: The maxToken value set to 8196 on the KG retrieval configuration
(symbol: maxToken) appears to be a typo; change it to 8192 (the standard 2^13
token limit) unless the +4 is intentional—if intentional, add an inline comment
next to the maxToken assignment explaining why the extra 4 tokens are required
so future readers understand the offset.
In `@internal/service/kg_types.go`:
- Around line 34-38: KGRelation (and the earlier KGEntity) are defined twice
which causes a redeclaration compile error; remove the duplicate type definition
from this file and reuse the single canonical type instead (or merge any
differing fields into one shared definition). Concretely: delete the KGRelation
declaration in this diff (or reconcile its fields with the existing
KGRelation/KGEntity declaration) so only one type named KGRelation exists;
update any local references to use that single definition and ensure
imports/builds compile against the unified type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0cd04be-97f6-4266-b44e-22da25057aad
📒 Files selected for processing (8)
internal/service/kg_pipeline.gointernal/service/kg_retrieval.gointernal/service/kg_retrieval_test.gointernal/service/kg_scoring_funcs.gointernal/service/kg_scoring_funcs_test.gointernal/service/kg_types.gointernal/service/tenant.gointernal/service/tenant_test.go
xugangqiang
force-pushed
the
feat/go-kg-retrieval
branch
from
e3ddf7d to
a0d0fe6
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/tenant.go (1)
793-795: ⚡ Quick winInconsistent nil-guard coverage across the service.
These nil guards for
userTenantDAOare only present inRemoveMemberandAcceptInvite, but at least 10 other methods in this service (GetTenantList,ListMembers,AddMember, etc.) call DAOs without similar guards. This creates an inconsistent defensive-programming pattern.Consider one of these approaches:
- Preferred (lower effort): Remove these guards and ensure
TenantServiceis always properly constructed viaNewTenantService()in both production and test code. Use proper mocking/initialization in tests rather than relying on nil checks.- Alternative: Apply nil guards consistently to all DAO/engine calls across the service if uninitialized service instances are a supported use case.
Partial nil-guard coverage creates maintenance confusion about which methods are "safe" to call on an uninitialized service.
Also applies to: 804-806
🤖 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 `@internal/service/tenant.go` around lines 793 - 795, The two one-off nil guards around userTenantDAO in RemoveMember and AcceptInvite create inconsistent behavior; remove those guards and ensure TenantService is always constructed with initialized DAOs via NewTenantService (set userTenantDAO and any other DAOs/engines there) and update tests to use NewTenantService or explicitly inject mocks (affects RemoveMember, AcceptInvite, GetTenantList, ListMembers, AddMember and other DAO-calling methods); alternatively, if uninitialized services must be supported, add the same nil-check pattern consistently to every method that uses DAOs instead of leaving only these two checks.
🤖 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.
Nitpick comments:
In `@internal/service/tenant.go`:
- Around line 793-795: The two one-off nil guards around userTenantDAO in
RemoveMember and AcceptInvite create inconsistent behavior; remove those guards
and ensure TenantService is always constructed with initialized DAOs via
NewTenantService (set userTenantDAO and any other DAOs/engines there) and update
tests to use NewTenantService or explicitly inject mocks (affects RemoveMember,
AcceptInvite, GetTenantList, ListMembers, AddMember and other DAO-calling
methods); alternatively, if uninitialized services must be supported, add the
same nil-check pattern consistently to every method that uses DAOs instead of
leaving only these two checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7a151fb-1d3e-434a-aa22-1dca184e91d0
📒 Files selected for processing (8)
internal/service/kg_pipeline.gointernal/service/kg_retrieval.gointernal/service/kg_retrieval_test.gointernal/service/kg_scoring_funcs.gointernal/service/kg_scoring_funcs_test.gointernal/service/kg_types.gointernal/service/tenant.gointernal/service/tenant_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/service/kg_types.go
- internal/service/tenant_test.go
- internal/service/kg_scoring_funcs.go
- internal/service/kg_pipeline.go
- internal/service/kg_scoring_funcs_test.go
- internal/service/kg_retrieval.go
- internal/service/kg_retrieval_test.go
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace e.Sim with e.Similarity for KGEntity - Add NhopEntity, Edge, EdgeScore, ScoredEntity, ScoredRelation to kg_search.go - Add Sim, PageRank fields to KGRelation - Fix test field references Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…thods Replace three inline go func closures with named Pipeline methods. Reduces Retrieval() visual noise by ~80 lines while keeping the parallelism pattern explicit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
size:XXL
xugangqiang
force-pushed
the
feat/go-kg-retrieval
branch
from
2b108fd to
e49a1a5
Compare
dosubot
Bot
added
size:XL
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
2 participants
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.
Summary
KGSearchRetrievalcomposes entity search, type search, relation search, N-hop analysis, score fusion, LLM-based query_rewrite, and community reports into a single synthetic chunk for KG-enhanced retrieval.Components
DocEngine.Searchcallscommon.AnalyzeNHopPaths/DoubleHitBoost/FuseRelationScorescommon.BuildQueryRewritePrompt/ParseQueryRewriteResponsecommon.BuildKGContent+NumTokensFromStringqueryRewritefunction with fallbackTesting
11 tests (pure function + mock engine):