Conversation
Due to high complexity, opencypher engine needs hard lifting
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 significantly refactors the OpenCypher query parsing and Abstract Syntax Tree (AST) processing within the engine. The changes aim to improve the modularity, readability, and extensibility of the query language implementation. By introducing a more structured approach to clause dispatching, statement construction, and expression parsing, the codebase becomes easier to maintain and extend. Additionally, a new AST rewriting framework is established to enable compile-time optimizations, and enhanced semantic analysis tools are integrated to provide more accurate validation and user-friendly error messages. 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
Activity
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 a936031. 🟢 All jobs passed!But CI Insights is watching 👀 |
Comprehensive PR Review: OpenCypher Parser RefactoringThis is a substantial refactoring of the OpenCypher parser components with excellent architectural improvements. The code is well-structured, documented, and includes comprehensive test coverage. 1. Code Quality & ArcadeDB Conventions ✅Strengths:
Minor Issues:
2. Potential Bugs
|
![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 is a major and impressive refactoring of the OpenCypher parser. The introduction of patterns like Strategy (ClauseDispatcher), Builder (StatementBuilder), and a dedicated utility class (ParserUtils) significantly improves the structure, readability, and maintainability of the code. Breaking down the monolithic CypherASTBuilder and CypherExpressionBuilder into smaller, more focused components is an excellent move. The addition of an AST rewriter framework with constant folding and normalization rules is a great foundation for future query optimizations. The new unit tests for the refactored components are also a very welcome addition. I've found a few minor issues, but overall this is a high-quality contribution.
| if (value.startsWith("'") && value.endsWith("'")) | ||
| return value.substring(1, value.length() - 1); | ||
|
|
||
| if (value.startsWith("\"") && value.endsWith("\"")) | ||
| return value.substring(1, value.length() - 1); |
There was a problem hiding this comment.
The parseValueString method does not decode escape sequences (like \n, \t) within string literals. It only strips the outer quotes. This will lead to incorrect string values when they are parsed from contexts that don't handle unescaping beforehand, such as the fallback parser in CypherASTBuilder. To ensure correctness, decodeStringLiteral should be called on the content of quoted strings.
| if (value.startsWith("'") && value.endsWith("'")) | |
| return value.substring(1, value.length() - 1); | |
| if (value.startsWith("\"") && value.endsWith("\"")) | |
| return value.substring(1, value.length() - 1); | |
| if (value.startsWith("'") && value.endsWith("'")) | |
| return decodeStringLiteral(value.substring(1, value.length() - 1)); | |
| if (value.startsWith("\"") && value.endsWith("\"")) | |
| return decodeStringLiteral(value.substring(1, value.length() - 1)); |
| final List<Expression> args = new java.util.ArrayList<>(); | ||
| args.add(new com.arcadedb.query.opencypher.ast.StarExpression()); | ||
| return new com.arcadedb.query.opencypher.ast.FunctionCallExpression("count", args, false); |
There was a problem hiding this comment.
The code uses fully qualified class names for java.util.ArrayList and AST classes like StarExpression and FunctionCallExpression. This makes the code unnecessarily verbose. Please add the corresponding import statements at the top of the file and use the simple class names instead.
| final List<Expression> args = new java.util.ArrayList<>(); | |
| args.add(new com.arcadedb.query.opencypher.ast.StarExpression()); | |
| return new com.arcadedb.query.opencypher.ast.FunctionCallExpression("count", args, false); | |
| final List<Expression> args = new ArrayList<>(); | |
| args.add(new StarExpression()); | |
| return new FunctionCallExpression("count", args, false); |
| private boolean isWrappedTrue(final BooleanExpression expr) { | ||
| if (expr instanceof BooleanWrapperExpression wrapper) { | ||
| // Check if wrapper contains literal true | ||
| // Note: BooleanWrapperExpression doesn't expose inner expression yet | ||
| // For now, return false - this will be implemented when wrapper API is available | ||
| return false; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The isWrappedTrue method (and similarly isWrappedFalse) will never work as intended. Its parameter expr has the type BooleanExpression, but the instanceof check is for BooleanWrapperExpression, which implements Expression, not BooleanExpression. Due to this type mismatch in the AST hierarchy, the check expr instanceof BooleanWrapperExpression will always evaluate to false, and the boolean simplification logic for literals will never be triggered. This part of the rewriter needs to be revisited to correctly handle boolean literals within logical expressions.
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 preferences |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3367 +/- ##
==========================================
- Coverage 61.03% 60.77% -0.26%
==========================================
Files 1164 1177 +13
Lines 79964 80654 +690
Branches 16036 16166 +130
==========================================
+ Hits 48805 49021 +216
- Misses 24248 24678 +430
- Partials 6911 6955 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.