Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, 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 introduces support for expression indexes within the Spanner DDL utility. By updating the core index column models and the schema scanner, the system can now correctly identify, parse, and represent indexes defined by expressions rather than simple column references. This enhancement ensures that data migration and schema conversion tools can handle modern Spanner index features, verified through updated unit tests for both supported SQL dialects. 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 the 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 counterproductive. 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. 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 support for expression-based index columns in Spanner DDL by adding an expression property to IndexColumn and updating InformationSchemaScanner to retrieve this field from the information schema. However, the current implementation contains several critical compilation and runtime issues. These include a missing @nullable annotation on the AutoValue expression() method, multiple syntax and type errors in IndexColumn.prettyPrint(), a casing typo on ResultSet.getString(), an invalid boolean check on a String variable, and trailing commas in the SQL select statements that will cause syntax errors at runtime.
| if (expression == null) { | ||
| appendable.append(quoteIdentifier(name(), dialect())); | ||
| } else { | ||
| appendable.append("(").append(expression.append(")"); | ||
| } | ||
| appendable..append(" ").append(order().title); |
There was a problem hiding this comment.
There are multiple compilation and syntax errors in this block:
expressionis an abstract method, so it must be called asexpression().expression.append(")")is invalid becauseStringdoes not have anappendmethod.appendable..appendcontains a double dot syntax error.
| if (expression == null) { | |
| appendable.append(quoteIdentifier(name(), dialect())); | |
| } else { | |
| appendable.append("(").append(expression.append(")"); | |
| } | |
| appendable..append(" ").append(order().title); | |
| if (expression() == null) { | |
| appendable.append(quoteIdentifier(name(), dialect())); | |
| } else { | |
| appendable.append("(\n").append(expression()).append(")"); | |
| } | |
| appendable.append(" ").append(order().title); |
| String indexLocalName = resultSet.getString(4); | ||
| String indexType = resultSet.getString(5); | ||
| String spannerType = resultSet.getString(6); | ||
| String expression = resultSet.GetString(7); |
There was a problem hiding this comment.
The method GetString does not exist on ResultSet. It should be lowercase getString.
| String expression = resultSet.GetString(7); | |
| String expression = resultSet.getString(7); |
| if (expression) { | ||
| indexColumnsBuilder.expression(expression); | ||
| } |
There was a problem hiding this comment.
In Java, a String cannot be used directly as a boolean condition in an if statement. You must check if it is not null.
| if (expression) { | |
| indexColumnsBuilder.expression(expression); | |
| } | |
| if (expression != null) { | |
| indexColumnsBuilder.expression(expression); | |
| } |
176981f to
4685cd6
Compare
addition: Add IMPORT/EXPORT support for expression indexes
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (25.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4455 +/- ##
============================================
+ Coverage 57.01% 57.03% +0.01%
- Complexity 7578 7581 +3
============================================
Files 1156 1156
Lines 73754 73761 +7
Branches 8698 8699 +1
============================================
+ Hits 42052 42066 +14
+ Misses 28900 28894 -6
+ Partials 2802 2801 -1
🚀 New features to boost your workflow:
|
No description provided.