#3811 remove Cypher support from gremlin#3814
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 100.00% diff coverage · -8.10% coverage variation
Metric Results Coverage variation ✅ -8.10% coverage variation Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (f642bb9) 116662 85638 73.41% Head commit (1efb01a) 147303 (+30641) 96192 (+10554) 65.30% (-8.10%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3814) 22 22 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
TIP This summary will be updated as you push new changes. Give us feedback
Code ReviewThis PR cleanly removes the Cypher-to-Gremlin translation bridge and its ~1800 lines of supporting code. The motivation is sound - the standalone OpenCypher engine is the right path forward. A few observations: Dead code in
|
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
There was a problem hiding this comment.
Code Review
This pull request removes the Cypher-to-Gremlin translation bridge and related classes, replacing them with a new opencypher engine implementation. The changes include updating test cases to use the new engine syntax, removing deprecated dependencies, and cleaning up initialization logic. I have identified two areas for improvement: the comment in ArcadeGraph.java regarding Cypher initialization is now inaccurate, and the commented-out merge() test in GremlinTest.java should be either refactored or removed to avoid dead code.
| importPlugin.classImports(Math.class); | ||
| importPlugin.methodImports(List.of("java.lang.Math#*")); |
There was a problem hiding this comment.
The comment // INITIALIZE CYPHER on line 516 is now misleading because the Cypher-to-Gremlin bridge has been removed from this module. This block now only initializes general Gremlin imports (such as Math). Consider updating the comment to reflect its current purpose or removing it entirely.
| // @Test | ||
| // void merge() { | ||
| // final ArcadeGraph graph = ArcadeGraph.open("./target/testgremlin"); | ||
| // try { | ||
| // graph.database.command("sqlscript",// | ||
| // "CREATE VERTEX TYPE TestMerge;" + // | ||
| // "CREATE PROPERTY TestMerge.id INTEGER;" +// | ||
| // "CREATE INDEX ON TestMerge (id) UNIQUE;"); | ||
| // | ||
| // graph.cypher("CREATE (v:TestMerge{id: 0})").execute(); | ||
| // graph.cypher("UNWIND range(0, 10) AS id MERGE (v:TestMerge{id: id}) RETURN v").execute(); | ||
| // | ||
| // } finally { | ||
| // graph.drop(); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
The merge() test case has been commented out. Leaving commented-out code in the repository is generally discouraged as it leads to dead code accumulation. If this test logic is still relevant for the new opencypher engine, it should be refactored to use that engine (e.g., via graph.getDatabase().command("opencypher", ...)). Otherwise, this block should be removed entirely.
|
Title: PR Review: Remove Cypher-to-Gremlin Bridge This is a clean, well-motivated removal of the old Cypher-to-Gremlin translation bridge (~1,800 lines deleted). The removal of the opencypher.gremlin:translation dependency also closes a security surface. Overall the change looks good. A few things worth discussing: Breaking Change - Backward Compatibility The removal of CypherQueryEngineFactory registration from QueryEngineManager.java means that any user calling database.query("cypher", ...) or database.command("cypher", ...) will now receive an error about an unknown engine. This is a user-visible breaking change with no migration path in this PR. Consider registering "cypher" as an alias that routes to "opencypher" in the new engine, or at minimum throwing a descriptive error message like: "Language 'cypher' has been replaced by 'opencypher'. Please update your code." rather than a generic "unknown language" error. Commented-Out Test In GremlinTest.java, the merge() test is commented out rather than deleted or rewritten. If this test case is no longer valid with the new engine, delete it. If it should still work, rewrite it using database.command("opencypher", ...) and re-enable it. Commented-out code should not be committed. GraphQLResultSet.java Fix The added fallback to look up the property directly from the element when not found in the projection is correct and necessary for the new OpenCypher engine. However, there is no explicit regression test covering this behavior change in GraphQLCypherDirectivesTest. The removal of the getExpectedPropertiesMetadata() override (which previously returned 0) suggests metadata properties are now correctly returned - a targeted test for this would prevent future regressions. Minor Nit The blank line left in gremlin/pom.xml after removing the translation dependency block can be cleaned up. Summary
The backward compatibility concern is the most important issue before merging - existing user code using "cypher" as the language name will silently break. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
After removing the Gremlin-based Cypher engine, the @cypher GraphQL directive now uses the native OpenCypher engine which returns results with the vertex set as element but properties wrapped in content map. GraphQLResultSet now falls back to reading properties from the element when not found in the projection content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
db014aa to
1efb01a
Compare
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
|
Code Review\n\nOverall, this is a well-motivated cleanup PR that removes the older Cypher-to-Gremlin translation bridge in favor of the native OpenCypher engine. The net result is 1887 deletions vs 98 additions - a significant reduction in complexity.\n\nPositive Aspects\n\n- Backward compatibility maintained: QueryEngineManager already registers 'cypher' as an alias for the OpenCypher engine (lines 62-65), so existing calls to database.command('cypher', ...) will continue to work with the native engine.\n- PostgreSQL wire protocol: PostgresNetworkExecutor already handles both 'cypher' and 'opencypher', so that path is fine.\n- Security improvement: Removes the Groovy-based translation path and the associated StringTranslationUtils injection-prevention workarounds.\n- Removes a complex, unmaintained dependency: The opencypher-gremlin translation library was a significant source of workarounds (expandAllKeysPattern, replaceParameterNames, ArcadeCustomFunctions).\n\n---\n\nIssues\n\n1. Commented-out test in GremlinTest.java (issue 1301): The merge() test was commented out rather than deleted or ported. This is a code smell. The test should either be rewritten using database.command('opencypher', ...) to verify the native engine handles the same scenario, or be deleted with a note explaining why it is no longer needed.\n\n2. Stale comment in QueryEngineManager.java: Line 67 still reads 'REGISTER GREMLIN AND ITS CYPHER ENGINE (OVERRIDES cypher WITH GREMLIN-BASED IMPLEMENTATION IF AVAILABLE)'. Since CypherQueryEngineFactory has been removed, the Gremlin module no longer overrides 'cypher'. The comment should be updated.\n\n3. GraphQLResultSet.java - why was getExpectedPropertiesMetadata() override removed? The new property fallback in mapProjections looks correct. However, getExpectedPropertiesMetadata() was removed from GraphQLCypherDirectivesTest without explanation - it previously returned 0 to accommodate the Gremlin-Cypher engine's different metadata behavior. A brief comment in the test or PR description explaining why the override is no longer needed would help future readers.\n\n4. Missing CHANGELOG / migration note: For a change of this scope - removing a query engine and a dependency - a CHANGELOG entry or migration note would be helpful. Key points: the gremlin-cypher bridge is removed (native OpenCypher is now the sole 'cypher' implementation), ArcadeGraph.cypher() API has been removed from the Gremlin module, the {cypher} prefix in the PostgreSQL wire protocol continues to work via the 'cypher' alias.\n\n---\n\nMinor: The large block of field reformatting in LocalDatabase.java (alignment style change) makes the diff harder to read. If this was triggered by Prettier/formatter, that is fine.\n\n---\n\nSummary: The core removal is correct and well-executed. The main actionable items are: (1) Delete or rewrite the commented-out merge() test in GremlinTest.java. (2) Update the stale comment on line 67 of QueryEngineManager.java. (3) Consider adding a CHANGELOG entry for the removal of ArcadeGraph.cypher(). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3814 +/- ##
==========================================
+ Coverage 64.56% 64.75% +0.18%
==========================================
Files 1582 1577 -5
Lines 116662 116168 -494
Branches 24741 24649 -92
==========================================
- Hits 75323 75219 -104
+ Misses 31052 30672 -380
+ Partials 10287 10277 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
bye bye |
|
I think you forgot to remove it from the frontend ! @robfrank |
|
do we have a frontend? :) |
|
Studio sounds very much like the frontend to me 
|
What does this PR do?
remvoes cypehr bridge from gremlin module
Motivation
the new OpenCypher support is mature
Related issues
#3811
Additional Notes
Checklist
mvn clean packagecommand