mysql8 #35
mysql8 #35
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @stadia, 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!
Summary of Changes
This pull request focuses on enhancing the ActiveRecord MySQL2RGeo adapter to support MySQL 8.0. The primary changes involve updating the test suite to include MySQL 8.0, modifying the schema creation process to handle SRID values correctly for MySQL 8.0, and adjusting the Arel visitor to include axis order in spatial queries for MySQL 8.0. Additionally, there are updates to RuboCop configurations and minor code adjustments for compatibility and code style.
Highlights
- MySQL 8.0 Support: Adds support for MySQL 8.0 by including it in the test matrix and adjusting schema creation and query generation.
- Schema Creation: Modifies the schema creation process to correctly handle SRID values when creating spatial columns in MySQL 8.0.
- Arel Visitor Updates: Updates the Arel visitor to include axis order in spatial queries for MySQL 8.0, ensuring correct spatial data handling.
- RuboCop Configuration: Updates the RuboCop configuration to reflect current code style and exclude certain files from specific checks.
Changelog
Click here to see the changelog
- .github/workflows/tests.yml
- Adds MySQL 8.0 to the test matrix to ensure compatibility.
- .rubocop_todo.yml
- Updates the RuboCop configuration with new generated config and excludes certain files from specific checks, such as Layout/HashAlignment and Naming/MethodParameterName.
- Removes some obsolete RuboCop configurations.
- activerecord-mysql2rgeo-adapter.gemspec
- Updates the rake development dependency to version 13.0 and adds rubocop as a development dependency.
- lib/active_record/connection_adapters/mysql2rgeo/arel_tosql.rb
- Refactors the
visit_Stringandvisit_in_spatial_contextmethods to use a newstring_querymethod. - Adds a
string_querymethod to include axis order in spatial queries for MySQL 8.0+.
- Refactors the
- lib/active_record/connection_adapters/mysql2rgeo/schema_creation.rb
- Modifies the
add_column_options!method to handle SRID values correctly for MySQL 8.0+.
- Modifies the
- lib/active_record/connection_adapters/mysql2rgeo/schema_statements.rb
- Removes the #:nodoc: from the indexes method definition.
- Removes unused variable assignment in
type_to_sqlmethod.
- lib/active_record/connection_adapters/mysql2rgeo/spatial_column.rb
- Corrects the initialization of SpatialColumn to properly set the limit when spatial and srid are present.
- lib/active_record/connection_adapters/mysql2rgeo_adapter.rb
- Changes
kind_of?tois_a?for checking array type. - Removes trailing comma in
mysql2rgeo_connectionmethod. - Updates
supports_spatial?to usedatabase_versioninstead ofversion. - Adds multiline support to the ST_GeomFromWKB generator.
- Changes
- lib/active_record/type/spatial.rb
- Changes
::String === valuetovalue.is_a?(::String)for type checking. - Uses
unpack1instead ofunpack("N").firstfor SRID parsing. - Removes unnecessary
-1index inparse_wktmethod.
- Changes
- lib/activerecord-mysql2rgeo-adapter.rb
- Updates the register method to use double quotes for string literals.
- test/ddl_test.rb
- Comments out the
test_create_geometry_using_shortcut_with_sridtest.
- Comments out the
- test/tasks_test.rb
- Uncomments and re-enables the
test_sql_dumpandtest_index_sql_dumptests. - Adds conditional check for database version in
test_basic_geography_schema_dump. - Re-enables the
test_add_index_with_no_optionstest.
- Uncomments and re-enables the
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 is currently in preview and 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 to provide feedback.
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.
Did you know?
MySQL was originally developed by a Swedish company called MySQL AB. It was later acquired by Sun Microsystems in 2008, which in turn was acquired by Oracle Corporation in 2010.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces support for MySQL 8.0 and includes updates to RuboCop configurations and gem dependencies. The changes appear to be well-structured and address the intended functionality. However, a few areas could benefit from further review and refinement.
Summary of Findings
- Code Duplication: The
string_querymethod inArel::Visitors::Mysql2Rgeoduplicates logic for parsing and constructing the ST_WKTToSQL query. Consolidating this logic can improve maintainability. - Conditional SRID Handling: The conditional logic for adding SRID in
add_column_options!inSchemaCreationcould be simplified for better readability. - Gem Dependency Update: The update of the
rakegem to version 13.0 is a good practice to keep dependencies up-to-date and secure.
Merge Readiness
The pull request is almost ready for merging. Addressing the code duplication in Arel::Visitors::Mysql2Rgeo and simplifying the conditional SRID handling in SchemaCreation would improve the code quality. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| def visit_String(node, collector) | ||
| node, srid = Mysql2Rgeo.parse_node(node) | ||
| collector << if srid == 0 | ||
| "#{st_func('ST_WKTToSQL')}(#{quote(node)})" | ||
| else | ||
| "#{st_func('ST_WKTToSQL')}(#{quote(node)}, #{srid})" | ||
| end | ||
| collector << string_query(node) | ||
| end |
There was a problem hiding this comment.
This logic is duplicated in visit_in_spatial_context. Consider extracting it into a separate method to avoid redundancy.
def visit_String(node, collector)
collector << string_query(node)
end| def visit_in_spatial_context(node, collector) | ||
| case node | ||
| when String | ||
| node, srid = Mysql2Rgeo.parse_node(node) | ||
| collector << if srid == 0 | ||
| "#{st_func('ST_WKTToSQL')}(#{quote(node)})" | ||
| else | ||
| "#{st_func('ST_WKTToSQL')}(#{quote(node)}, #{srid})" | ||
| end | ||
| collector << string_query(node) |
There was a problem hiding this comment.
This logic is duplicated in visit_String. Consider extracting it into a separate method to avoid redundancy.
def visit_in_spatial_context(node, collector)
case node
when String
collector << string_query(node)| sql << if @conn.database_version >= "8.0.0" | ||
| " SRID #{options[:srid]} " | ||
| else | ||
| " /*!80003 SRID #{options[:srid]} */" | ||
| end |
There was a problem hiding this comment.
Consider simplifying this conditional logic. You could directly use the appropriate string based on the version check, instead of using an if statement.
sql << (@conn.database_version >= "8.0.0" ? " SRID #{options[:srid]} " : " /*!80003 SRID #{options[:srid]} */")
No description provided.