Feature/gql parser #619
Feature/gql parser #619
Conversation
Summary of ChangesHello @sandrawar, 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 integrates a new Graph Query Language (GQL) parser using ANTLR4, establishing the foundation for processing GQL queries within the system. Concurrently, it significantly enhances the Gremlin Highlights
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
|
![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 new GQL parser using ANTLR and refactors the Gremlin command step to be chainable. The changes include adding the ANTLR maven plugin, the GQL grammar file, and extensive tests for the new parser. The command step is improved to support chaining, which enhances the DSL's usability for transactional operations. The related services and transaction handling logic have been updated accordingly. The changes are well-structured and include new integration tests for the command chaining feature. My feedback includes suggestions to fix typos in the grammar for better maintainability, improve test robustness by failing fast on missing test data, and centralize dependency version management in the pom.xml.
core/pom.xml
Outdated
| <groupId>org.antlr</groupId> | ||
| <artifactId>antlr4-maven-plugin</artifactId> | ||
| <version>4.13.2</version> |
There was a problem hiding this comment.
The ANTLR version 4.13.2 is hardcoded here. The same version is used for the antlr4-runtime dependency in the root pom.xml. To ensure these versions stay in sync and to make future updates easier, consider defining an antlr.version property in the root pom.xml and using ${antlr.version} in both places.
core/src/main/grammar/antlr/GQL.g4
Outdated
| simple_linear_query_statement: (primitive_query_statement)* return_statment?; | ||
| primitive_query_statement: call_statment | filter_statment | for_statment | let_statmnet | | ||
| limit_statment | match_statment | offset_statment | order_by_statment | | ||
| skip_statment | with_statmnet; |
There was a problem hiding this comment.
There are typos in several parser rule names (e.g., return_statment, let_statmnet, with_statmnet). They should be corrected to end with _statement for consistency and to generate more intuitive parser methods. Please apply this correction throughout the file where these rules are defined and used.
simple_linear_query_statement: (primitive_query_statement)* return_statement?;
primitive_query_statement: call_statment | filter_statment | for_statment | let_statement |
limit_statment | match_statment | offset_statment | order_by_statment |
skip_statment | with_statement;
| if (!Files.exists(path)) { | ||
| System.err.println("WARNING: Directory does not exist: " + path); | ||
| return Stream.empty(); | ||
| } |
There was a problem hiding this comment.
If the directory with test files doesn't exist, the test will silently pass because an empty stream is returned. This could hide issues in the build or test setup. It would be more robust to fail the test explicitly if the directory is not found.
if (!Files.exists(path)) {
throw new java.nio.file.NoSuchFileException("Test data directory not found: " + path);
}
|
Hi @sandrawar |
sandrawar
force-pushed
the
feature/gql-parser
branch
from
bc47293 to
8ac3f45
Compare
andrii0lomakin
left a comment
andrii0lomakin
left a comment
There was a problem hiding this comment.
Very good, but requires some changes.
| /// | ||
| /// @param command The SQL command to execute. | ||
| /// @return A traversal that can be chained with other steps. | ||
| public <S> GraphTraversal<S, S> sqlCommand(@Nonnull String command) { |
There was a problem hiding this comment.
I do like quaility of comments.
| /// @param command The SQL command to execute. | ||
| /// @param arguments The arguments to pass to the command. | ||
| /// @return A traversal that can be chained with other steps. | ||
| public <S> GraphTraversal<S, S> sqlCommand(@Nonnull String command, @Nonnull Map<?, ?> arguments) { |
There was a problem hiding this comment.
This method is not tested in Gherkin tests , also I propose to use pairs of string, value as varargs here.
|
|
||
| try { | ||
| session.command(command, params); | ||
| if (startedTx) { |
There was a problem hiding this comment.
We should not autocommit after each session it is controlled by the user
| || normalized.startsWith("DROP PROPERTY") | ||
| || normalized.startsWith("CREATE INDEX") | ||
| || normalized.startsWith("DROP INDEX") | ||
| || normalized.startsWith("CREATE VERTEX") |
There was a problem hiding this comment.
Not schema command but DML command
| || normalized.startsWith("CREATE INDEX") | ||
| || normalized.startsWith("DROP INDEX") | ||
| || normalized.startsWith("CREATE VERTEX") | ||
| || normalized.startsWith("CREATE EDGE"); |
| if ("COMMIT".equals(normalized)) { | ||
| if (session.isTxActive()) { | ||
| session.commit(); | ||
| } |
There was a problem hiding this comment.
We should throw an exception if we commit TX but it does not exist.
| try (var session = acquireSession()) { | ||
| session.command(command, params); | ||
| var normalized = command == null ? "" : command.trim().toUpperCase(); | ||
| if ("BEGIN".equals(normalized)) { |
There was a problem hiding this comment.
This command does not have sense as closing session will cause to rollback tx
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
…pattern) implemented
…s the schema for next runds (to be able to chceck if we didn't mess up grammar while changing it)
andrii0lomakin
force-pushed
the
feature/gql-parser
branch
from
70a3314 to
d90f429
Compare
| BOOL: T R U E | F A L S E; | ||
| DOT : '.' ; | ||
| DASH: '-'; | ||
| ID: [a-zA-Z_][a-zA-Z_0-9]* ; |
There was a problem hiding this comment.
We use different ID syntax, namely #d+:d+, we should change it, to properly support value expressions.
| comparison_operator: EQ | NEQ | GT | GTE | LT | LTE | IN; | ||
| sub: DASH; | ||
| numeric_literal: (sub)? (NUMBER | INT); | ||
| property_reference : ID (DOT ID)* ; |
There was a problem hiding this comment.
With such defenition of ID as I mentioned, that would be incorrect.
|
|
||
| private static boolean isSchemaCommand(String normalized) { | ||
| return normalized.startsWith("CREATE CLASS") | ||
| || normalized.startsWith("DROP CLASS") |
There was a problem hiding this comment.
Alter statements are missed, like ALTER INDEX for example. Please relace manual detection by calling of SQL parser and the check instanceof DDLSStatement
| && !argsList.isEmpty()) { | ||
| if (argsList.getFirst() instanceof String cmd) { | ||
| finalCommand = cmd; | ||
| if (argsList.size() > 1) { |
There was a problem hiding this comment.
There is no check that arguments are provided in pairs.
| return; | ||
| } | ||
| case "ROLLBACK" -> { | ||
| var tx = tx(); |
There was a problem hiding this comment.
There is no check that TX is present it should be the same as for commit.
| var path = Paths.get(pathStr).toAbsolutePath(); | ||
| if (!Files.exists(path)) { | ||
| System.err.println("WARNING: Directory does not exist: " + path); | ||
| return Stream.empty(); |
There was a problem hiding this comment.
Please fix Gemini observation
| } | ||
|
|
||
| static Stream<Path> getPositiveGqlFiles() throws IOException { | ||
| return getFilesFromPath("src/test/resources/gql-tests/positive"); |
There was a problem hiding this comment.
Please use getClass().getResource() to avoid fix of realtive path to the current directory.
Motivation:
This PR introduces:
Changes:
ANTLR4 grammar defining GQL syntax with support for:
Note: This PR only includes the grammar definition and parsing tests. Query execution will be implemented in a future PR.
YTDBGraphTraversalSourceDSL:
YTDBCommandService:
Usage:
// Single command (eager)g().command("CREATE CLASS Person EXTENDS V");
// Chaining (lazy) g().sqlCommand("BEGIN").sqlCommand("INSERT INTO...").sqlCommand("COMMIT").iterate();