- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.5k
 
feature(spanner): Implement DML Support for Spanner #1197
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
base: master
Are you sure you want to change the base?
feature(spanner): Implement DML Support for Spanner #1197
Conversation
c60aa87    to
    be12225      
    Compare
  
    | 
          
 This PR supersedes the following PR #733, #910, #924 It is a robust implementation that greatly improves support for Spanner migrations. It also fixes a lot of open issues with the existing DDL parsing implementation. NOTE: Not sure why tests are failing, but it is not related to spanner tests. It looks like cockroachdb is having an issue.  | 
    
| 
           FYI: Now memefish has official release, so you don't need to use   | 
    
| 
           @dhui  | 
    
ed3767b    to
    07e9f94      
    Compare
  
    | 
           Hello, any update on this feature ?  | 
    
| 
          
 Will you please take a look at this PR? It has been well tested and is in production use, so would be nice to get it merged!  | 
    
| 
           @dhui googleapis/google-cloud-go#11941 spansql package is  
 Almost all of the issues with Spanner in golang-migrate are caused by spansql. What is the reason for not responding to this PR?  | 
    
07e9f94    to
    dd4885a      
    Compare
  
    | 
          
 Rebased onto master branch and all tests look good. I would love to get a review on this!  | 
    
        
          
                database/spanner/examples/migrations2/1481574547_create_users_table.down.sql
              
                Outdated
          
            Show resolved
            Hide resolved
        
      548bc4c    to
    4819a3a      
    Compare
  
    4819a3a    to
    85cce1e      
    Compare
  
    | 
          
 Rebased onto master branch and all tests look good. I would love to get a review on this!  | 
    
| ## DDL with comments | ||
| ## DDL & DML with comments | ||
| At the moment the GCP Spanner backed does not seem to allow for comments (See https://issuetracker.google.com/issues/159730604) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI
Both of the real Spanner instance and cloud-spanner-emulator(https://github.com/GoogleCloudPlatform/cloud-spanner-emulator/releases/tag/v1.5.31) now accepts DDLs with comments.
| }, | ||
| }, | ||
| { | ||
| name: "multi statement, inline comment inside DML", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that multiStatement only has DDLs(CREATE TABLE, CREATE INDEX)
Closes #135
Closes #775
Closes #918
Closes #1194
Supersedes the following PRs #733, #910, #924, #1287
This PR implements DML support and uses the
memefishlexer to parse the migrations, replacing the usage of thespansqlpackage. As a result, numerous otherspansqlrelated issues have been resolved.The following features have been implemented:
To maintain backward compatibility, the
x-clean-statementsoption must be used to enable the above features.This PR also fixes a bug where a provided spanner client via
WithInstance()is closed whenMigration.Close()is called. According to Docs, the caller is responsible for closing the underlying database client. This fix brings the Spanner driver into compliance with the expected behavior.I am maintaining a fork with this fix for those who would like to use it.
Put the following replace directive you your projects
go.mod