Skip to content

[FEATURE] Add backend-neutral GraphQueryOperation support - #491

Open
KilianTrunk wants to merge 1 commit into
awslabs:mainfrom
Ortecha:feat/graph-query-operation
Open

[FEATURE] Add backend-neutral GraphQueryOperation support#491
KilianTrunk wants to merge 1 commit into
awslabs:mainfrom
Ortecha:feat/graph-query-operation

Conversation

@KilianTrunk

Copy link
Copy Markdown

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 GraphQueryOperation and 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

  • Add and publicly export the GraphQueryOperation enum with 32 lexical-graph read and write operations.
  • Add an optional operation argument to GraphStore.execute_query() and GraphStore.execute_query_with_retry().
  • Add the _execute_operation() dispatch hook for graph stores that provide operation-specific implementations.
  • Preserve existing graph-store behavior through the default _execute_operation() implementation, which delegates to _execute_query().
  • Propagate operation context through retries, Query, QueryTree, Job, GraphBatchClient, and MultiTenantGraphStore.
  • Forward tenant context to operation-specific implementations while preserving the existing native-query calling convention.
  • Assign semantic operations across graph builders, the in-graph chunk store, retrievers, and query-context providers.
  • Forward backend-neutral parameters required to preserve existing behavior, including entity classifications, chunk relationship types, local-entity literal values, and includeChunkMetadata.
  • Preserve the existing public include_chunk_details option while exposing its value to operation-specific backends as includeChunkMetadata.
  • Simplify correlation-ID handling so an omitted or empty value produces only the generated ID, while a supplied value produces <supplied>/<generated>.
  • Add and update unit tests for operation dispatch, fallback behavior, batching, query trees, tenant propagation, chunk-store forwarding, and correlation-ID generation.

Problem

The existing GraphStore API 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

  • Unit tests added/updated
  • Integration tests added (existing integration tests and targeted E2E scenarios were used)
  • Existing tests pass (pytest)
  • Tested manually as described below

Validation performed:

  • Ran the complete test suite on Python 3.10, 3.11, and 3.12:
    • 2,002 passed
    • 15 skipped
    • 0 failed
    • 64.31% coverage against the required 56%
  • Repeated the Python 3.12 suite in a clean, CI-equivalent environment:
    • 2,002 passed
    • 15 skipped
    • 0 failed
  • Ran the existing integration suite against a live Neo4j 5 container:
    • 8 passed
    • 7 AWS-dependent tests skipped
    • 0 failed
  • Ran an end-to-end lexical-graph build against live Neo4j, exercising all graph builders modified by this PR.
  • Ran the same build against the parent commit and confirmed identical graph node and relationship counts.
  • Ran end-to-end traversal-based retrieval against live Neo4j and pgvector and confirmed that results were returned.
  • Ran a multi-tenant build and traversal retrieval and confirmed that tenant-scoped labels and queries worked correctly.
  • Specifically exercised the modified QueryTree, Job, GraphBatchClient, and MultiTenantGraphStore forwarding paths.
  • Verified that existing graph stores preserve their previous behavior through the default _execute_operation() fallback.
  • Ran license-header validation: 0 files missing headers.
  • git diff --check upstream/main..HEAD: no errors.
  • Merge-tree simulation against the current upstream/main: no conflicts.

Checklist

  • Code follows existing style and conventions
  • License headers present on new files
  • Documentation updated (not applicable to this core-only change; user-facing SPARQL documentation is included in the follow-up contributor PR)
  • No breaking changes

GraphQueryOperation is 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.

Co-authored-by: Matic <majericmatic@gmail.com>
Co-authored-by: Tom Soru <tom@tommaso-soru.it>
@acarbonetto

Copy link
Copy Markdown
Collaborator

Thank you for this. I will take a look at this asap.

@mykola-pereyma mykola-pereyma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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_provider currently tags get_statements_for_topic(topic_id) with GET_TOPIC. That reads as a post-VSS fetch-by-id (returns statements for an already-resolved topic id), so GET_TOPIC is defensible — but if the intent was the topic search entry point, SEARCH_BY_TOPIC may belong there. Please confirm which is correct.
  2. 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.
  3. 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants