Skip to content

Grammar fixes #236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Grammar fixes #236

wants to merge 9 commits into from

Conversation

chedim
Copy link
Collaborator

@chedim chedim commented Aug 15, 2025

No description provided.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @chedim, 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 primarily focuses on updating the project's build infrastructure and making significant refinements to the SQL++ grammar. Key changes include upgrading the IntelliJ Platform Gradle plugin and Gradle wrapper, along with minor adjustments to Gradle wrapper scripts. The most substantial modifications involve a comprehensive overhaul of the SQL++ parser, streamlining expression handling, integrating the BETWEEN clause directly into relational expressions, and revising comment parsing.

Highlights

  • Build Tool Updates: Upgraded the IntelliJ Platform Gradle plugin to version 2.7.1 and the Gradle wrapper to 9.0.0, ensuring compatibility with newer IntelliJ IDEA versions.
  • Gradle Script Refinements: Enhanced gradlew and gradlew.bat scripts by updating copyright information, adding SPDX license identifiers, and improving environment variable handling for APP_HOME and CLASSPATH.
  • SQL++ Grammar Modernization: Standardized expression parsing by replacing cond with expr across numerous grammar rules, simplifying the language structure.
  • BETWEEN Clause Integration: The BETWEEN clause is now directly handled within relational_expr, removing the need for a separate between_expr rule.
  • Comment Parsing Adjustments: Revised the parsing of block comments, removing redundant grammar rules and updating hint comment structures.
  • New Array Expression Support: Introduced new-array-expr to allow for the creation of array literals within expressions.
  • Standardized Option Handling: Replaced various options references with a more generic obj type in grammar rules related to ansi_merge_insert, infer_statement, insert_select, and values_clause.
  • Index Reference Consistency: Updated indexes_property to consistently use index-ref for index references.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several grammar fixes for SQL++, notably for the BETWEEN clause and for comments. It also updates Gradle and several dependencies to more recent versions. The changes to the generated parser files reflect the grammar modifications. My review focuses on the non-generated files. I've identified a potential issue in one of the test files regarding mixed JUnit versions and a fragile regular expression in the lexer definition that should be addressed.

@@ -42,7 +40,7 @@ public class DataLoaderTest {
private static String secondaryWithDesc = "secondaryWithDesc";


@Container
@Rule

Choose a reason for hiding this comment

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

high

You're mixing JUnit 4 and JUnit 5 annotations here. The test class uses JUnit 5 annotations like @BeforeAll and @Test, but you've replaced @Container (a JUnit 5 Testcontainers annotation) with @Rule (a JUnit 4 Testcontainers annotation). This can lead to unpredictable behavior or tests not running correctly.

To ensure consistency and correctness, you should either:

  1. Use the JUnit 5 Testcontainers annotations (@Testcontainers on the class and @Container on the field).
  2. Migrate the entire test class to use JUnit 4 annotations.

Given that the rest of the test class uses JUnit 5, sticking with the JUnit 5 approach for Testcontainers seems more appropriate.

Suggested change
@Rule
@Container

@@ -44,6 +44,10 @@ BACKSLASH = \\
QUOTE = \'
DQUOTE = \"
DOLLAR = "$"
LINE_COMMENT="//" .*
BLOCK_COMMENT="/*" [^+] !([^]* "*/" [^]*) ("*/")?

Choose a reason for hiding this comment

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

high

The regular expression for BLOCK_COMMENT appears to be incorrect and fragile.

  • The [^+] part prevents any block comment from starting with /*+, which is likely intended to allow BLOCK_HINT_OPEN to match. A more robust approach is to define the rule for BLOCK_HINT_OPEN before the rule for BLOCK_COMMENT, as JFlex uses the first matching rule.
  • The !([^]* "*/" [^]*) part is not standard JFlex syntax and its behavior is unclear. It seems to be an attempt at non-greedy matching, which can be brittle.

A more standard and robust way to define a non-nested block comment in JFlex is:
"/*" ( [^*] | \*+[^*/] )* \*+ "/"

I recommend reordering the rules in the %% block to prioritize hints and using a more standard regular expression for block comments.

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.

1 participant