Conversation
Reached much better compliance with the latest changes:
```
=======================================================
OpenCypher TCK Compliance Report
=======================================================
Total scenarios: 3897
Passed: 1908 (48%)
Failed: 1939 (49%)
Skipped: 50 (1%)
-------------------------------------------------------
By category:
clauses/call 2/ 52 passed (3%)
clauses/create 64/ 78 passed (82%)
clauses/delete 24/ 41 passed (58%)
clauses/match 292/381 passed (76%)
clauses/match-where 25/ 34 passed (73%)
clauses/merge 47/ 75 passed (62%)
clauses/remove 29/ 33 passed (87%)
clauses/return 35/ 63 passed (55%)
clauses/return-orderby 23/ 35 passed (65%)
clauses/return-skip-limit 26/ 31 passed (83%)
clauses/set 30/ 53 passed (56%)
clauses/union 8/ 12 passed (66%)
clauses/unwind 10/ 14 passed (71%)
clauses/with 14/ 29 passed (48%)
clauses/with-orderBy 124/292 passed (42%)
clauses/with-skip-limit 7/ 9 passed (77%)
clauses/with-where 10/ 19 passed (52%)
expressions/aggregation 23/ 35 passed (65%)
expressions/boolean 150/150 passed (100%)
expressions/comparison 36/ 72 passed (50%)
expressions/conditional 13/ 13 passed (100%)
expressions/existentialSubqueries 4/ 10 passed (40%)
expressions/graph 32/ 61 passed (52%)
expressions/list 120/185 passed (64%)
expressions/literals 120/131 passed (91%)
expressions/map 28/ 44 passed (63%)
expressions/mathematical 3/ 6 passed (50%)
expressions/null 44/ 44 passed (100%)
expressions/path 0/ 7 passed (0%)
expressions/pattern 19/ 50 passed (38%)
expressions/precedence 20/121 passed (16%)
expressions/quantifier 478/604 passed (79%)
expressions/string 22/ 32 passed (68%)
expressions/temporal 0/1004 passed (0%)
expressions/typeConversion 19/ 47 passed (40%)
useCases/countingSubgraphMatches 6/ 11 passed (54%)
useCases/triadicSelection 1/ 19 passed (5%)
=======================================================
```
Summary of ChangesHello @lvca, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant batch of fixes and improvements aimed at enhancing the OpenCypher implementation's adherence to the TCK specification. The changes span across various core components, including expression evaluation, temporal data handling, query clause processing, and semantic validation. The primary goal is to ensure more accurate and compliant behavior for a wider range of Cypher queries, particularly those involving complex data types, arithmetic operations, and data manipulation statements. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🧪 CI InsightsHere's what we observed from your CI run for 900a178. 🟢 All jobs passed!But CI Insights is watching 👀 |
Code Review: OpenCypher TCK Test FixesThis PR implements important fixes for OpenCypher TCK test compliance. I've reviewed the changes and have the following feedback: ✅ Strengths
🔍 Issues & SuggestionsCritical
Moderate
Minor
🧪 Testing Recommendations
📝 Code Style
✏️ Nitpicks
🎯 RecommendationApprove with minor revisions. The critical issues should be addressed before merge:
The other issues are improvements that could be addressed in follow-up work. Review generated following ArcadeDB project guidelines from CLAUDE.md |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a substantial number of fixes and improvements to the OpenCypher engine, aimed at increasing compliance with the TCK tests. The changes are wide-ranging, including:
- Enhanced
SETclause functionality to support map replacement/merging and label setting. - Implementation of lexicographical list comparison.
- Addition of new functions like
substring,percentileDisc, andpercentileCont. - Numerous correctness fixes for temporal type handling, arithmetic operations, and semantic validation.
The code quality is generally high, and the changes significantly improve the engine's capabilities. I've provided a few suggestions for refactoring to improve efficiency and reduce code duplication. Overall, this is a great contribution.
| for (int i = 0; i < minSize; i++) { | ||
| final Object elemCmp = new ComparisonExpression( | ||
| new LiteralExpression(leftList.get(i), ""), Operator.EQUALS, | ||
| new LiteralExpression(rightList.get(i), "")) | ||
| .evaluateTernary(null, null); | ||
| if (elemCmp == null) | ||
| return null; // null element makes ordering undefined | ||
| if (Boolean.TRUE.equals(elemCmp)) | ||
| continue; // elements are equal, compare next | ||
| // Elements differ: check less than | ||
| final Object ltResult = new ComparisonExpression( | ||
| new LiteralExpression(leftList.get(i), ""), Operator.LESS_THAN, | ||
| new LiteralExpression(rightList.get(i), "")) | ||
| .evaluateTernary(null, null); | ||
| if (ltResult == null) | ||
| return null; | ||
| final boolean isLess = Boolean.TRUE.equals(ltResult); | ||
| return switch (operator) { | ||
| case LESS_THAN -> isLess; | ||
| case GREATER_THAN -> !isLess; | ||
| case LESS_THAN_OR_EQUAL -> isLess; | ||
| case GREATER_THAN_OR_EQUAL -> !isLess; | ||
| default -> null; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The implementation for lexicographical list comparison creates new ComparisonExpression and LiteralExpression objects within a loop for each element comparison. This is inefficient and can lead to significant overhead and performance degradation, especially for large lists.
To improve performance, this logic should be refactored to avoid unnecessary object instantiation. Consider extracting the comparison logic into a helper method that can be called directly with the list elements, or by making the compareValuesTernary method accessible for recursive calls without creating new ComparisonExpression instances.
| final List<String> labels = new ArrayList<>(superTypes.size()); | ||
| for (final DocumentType superType : superTypes) | ||
| labels.add(superType.getName()); | ||
| for (final DocumentType superType : superTypes) { | ||
| final String name = superType.getName(); | ||
| if (!"Vertex".equals(name) && !"V".equals(name)) | ||
| labels.add(name); | ||
| } | ||
| Collections.sort(labels); | ||
| return labels; |
There was a problem hiding this comment.
This block of code for filtering and sorting labels can be made more concise and expressive by using Java Streams. This would improve readability and align with modern Java practices.
| final List<String> labels = new ArrayList<>(superTypes.size()); | |
| for (final DocumentType superType : superTypes) | |
| labels.add(superType.getName()); | |
| for (final DocumentType superType : superTypes) { | |
| final String name = superType.getName(); | |
| if (!"Vertex".equals(name) && !"V".equals(name)) | |
| labels.add(name); | |
| } | |
| Collections.sort(labels); | |
| return labels; | |
| // Multi-label vertex - supertypes are the labels, filter out base types | |
| return superTypes.stream() | |
| .map(DocumentType::getName) | |
| .filter(name -> !"Vertex".equals(name) && !"V".equals(name)) | |
| .sorted() | |
| .collect(java.util.stream.Collectors.toList()); |
| * | ||
| * @param result the result containing variables to update | ||
| */ | ||
| private void applySetOperations(final Result result) { |
There was a problem hiding this comment.
The logic for applying SET operations in this class, particularly in applySetOperations and its helper methods (applyPropertySet, applyReplaceMap, applyMergeMap, applyLabels), is very similar to the logic found in MergeStep.applySetClause. This code duplication can make future maintenance more difficult, as changes would need to be applied in both places.
Consider refactoring this common logic into a shared utility class or methods within the package to reduce duplication and improve maintainability.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3366 +/- ##
==========================================
- Coverage 61.28% 60.49% -0.80%
==========================================
Files 1157 1164 +7
Lines 78083 79962 +1879
Branches 15444 16034 +590
==========================================
+ Hits 47855 48370 +515
- Misses 23398 24682 +1284
- Partials 6830 6910 +80 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* test: including official opencypher tck
* fix: fixed many issues with opencypher from TCK tests
* fix: fixed broken tests from OpenCyphr TCK
* fix: more opencypher issues from tck tests
* fix: opencypher tck more tests pass now
Reached much better compliance with the latest changes:
```
=======================================================
OpenCypher TCK Compliance Report
=======================================================
Total scenarios: 3897
Passed: 1908 (48%)
Failed: 1939 (49%)
Skipped: 50 (1%)
-------------------------------------------------------
By category:
clauses/call 2/ 52 passed (3%)
clauses/create 64/ 78 passed (82%)
clauses/delete 24/ 41 passed (58%)
clauses/match 292/381 passed (76%)
clauses/match-where 25/ 34 passed (73%)
clauses/merge 47/ 75 passed (62%)
clauses/remove 29/ 33 passed (87%)
clauses/return 35/ 63 passed (55%)
clauses/return-orderby 23/ 35 passed (65%)
clauses/return-skip-limit 26/ 31 passed (83%)
clauses/set 30/ 53 passed (56%)
clauses/union 8/ 12 passed (66%)
clauses/unwind 10/ 14 passed (71%)
clauses/with 14/ 29 passed (48%)
clauses/with-orderBy 124/292 passed (42%)
clauses/with-skip-limit 7/ 9 passed (77%)
clauses/with-where 10/ 19 passed (52%)
expressions/aggregation 23/ 35 passed (65%)
expressions/boolean 150/150 passed (100%)
expressions/comparison 36/ 72 passed (50%)
expressions/conditional 13/ 13 passed (100%)
expressions/existentialSubqueries 4/ 10 passed (40%)
expressions/graph 32/ 61 passed (52%)
expressions/list 120/185 passed (64%)
expressions/literals 120/131 passed (91%)
expressions/map 28/ 44 passed (63%)
expressions/mathematical 3/ 6 passed (50%)
expressions/null 44/ 44 passed (100%)
expressions/path 0/ 7 passed (0%)
expressions/pattern 19/ 50 passed (38%)
expressions/precedence 20/121 passed (16%)
expressions/quantifier 478/604 passed (79%)
expressions/string 22/ 32 passed (68%)
expressions/temporal 0/1004 passed (0%)
expressions/typeConversion 19/ 47 passed (40%)
useCases/countingSubgraphMatches 6/ 11 passed (54%)
useCases/triadicSelection 1/ 19 passed (5%)
=======================================================
```
* fix: opencypher implemented missing temporal functions + precedence
Issue #3357 and #3355
* fix: opencypher implemented more missing functions
Issue #3357 and #3355
* fix: opencypher more code fixed thank to the tck
(cherry picked from commit 935cb85)
1 participant
No description provided.