[FEATURE] Add backend-neutral GraphQueryOperation support - #491
[FEATURE] Add backend-neutral GraphQueryOperation support#491KilianTrunk wants to merge 1 commit into
Conversation
Co-authored-by: Matic <majericmatic@gmail.com> Co-authored-by: Tom Soru <tom@tommaso-soru.it>
|
Thank you for this. I will take a look at this asap. |
mykola-pereyma
left a comment
There was a problem hiding this comment.
Really solid, well-tested feature — the abstraction is cleanly additive and backward compatibility holds (optional operation, default _execute_operation() delegating to _execute_query(), native queries/params unchanged for the default backend, and the E2E identical node/relationship counts vs the parent commit are convincing).
I did a per-assignment semantic audit of all 32 operations against the queries they tag. 31 are assigned and accurate — including the easy-to-swap FIND_COMPLEMENTS / FIND_SUBJECTS pair (verified correct against the query comments and the get-complements-for-subject / get-real-subjects-for-complement tree names), and entity_vss_provider's topic/chunk branch (FIND_ENTITIES_BY_TOPICS / FIND_ENTITIES_BY_CHUNKS).
Requesting changes on one point before approval, since the enum is now a public, exported API contract:
SEARCH_BY_TOPIC is defined but never assigned
GraphQueryOperation.SEARCH_BY_TOPIC has no call site anywhere in the PR. Every other value is wired to a real query and exercised; this one is inert. Shipping an unused member in a public contract is a maintenance hazard — a future backend implementer will assume every value has a well-defined, exercised meaning.
Please do one of the following:
- Assign it if a topic-search site was missed, and add a unit test that dispatches through it (mirroring the operation-dispatch tests you already added). Candidate to double-check:
keyword_vss_providercurrently tagsget_statements_for_topic(topic_id)withGET_TOPIC. That reads as a post-VSS fetch-by-id (returns statements for an already-resolved topic id), soGET_TOPICis defensible — but if the intent was the topic search entry point,SEARCH_BY_TOPICmay belong there. Please confirm which is correct. - Document it as an intentional placeholder — a short comment on the enum member explaining the future/alternative-backend use case it's reserved for, so it's clearly deliberate rather than an oversight.
- Remove it to keep the contract minimal until there's a consumer.
Everything else looks good; happy to approve once SEARCH_BY_TOPIC is resolved (assigned + tested, documented, or removed).
Description
This PR adds backend-neutral, typed graph operation identifiers to
lexical-graph.Graph builders and retrievers continue to construct their existing native queries, but now also pass an optional
GraphQueryOperationand structured parameters. This allows graph stores using another query language to select an equivalent native implementation without parsing openCypher.This is the core-only split requested during the review of #405. It does not include the SPARQL contributor package or development-environment changes.
Changes
GraphQueryOperationenum with 32 lexical-graph read and write operations.operationargument toGraphStore.execute_query()andGraphStore.execute_query_with_retry()._execute_operation()dispatch hook for graph stores that provide operation-specific implementations._execute_operation()implementation, which delegates to_execute_query().Query,QueryTree,Job,GraphBatchClient, andMultiTenantGraphStore.includeChunkMetadata.include_chunk_detailsoption while exposing its value to operation-specific backends asincludeChunkMetadata.<supplied>/<generated>.Problem
The existing
GraphStoreAPI only exposed a native query string and its parameters.That works for graph stores using the same query language, but an alternative backend cannot reliably determine the semantic intent of generated openCypher without parsing it.
This change adds an optional semantic operation contract while preserving the existing execution path for current graph stores.
Related issue (if any): N/A
Testing
pytest)Validation performed:
QueryTree,Job,GraphBatchClient, andMultiTenantGraphStoreforwarding paths._execute_operation()fallback.git diff --check upstream/main..HEAD: no errors.upstream/main: no conflicts.Checklist
GraphQueryOperationis optional. Existing graph stores that do not implement operation-specific execution continue to execute their existing native queries through_execute_query().By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.