-
Notifications
You must be signed in to change notification settings - Fork 3
Implement BEGIN [RW] ISOLATION LEVEL #174
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |
| "strings" | ||
| "time" | ||
|
|
||
| sppb "cloud.google.com/go/spanner/apiv1/spannerpb" | ||
| "github.com/apstndb/gsqlutils/stmtkind" | ||
| "github.com/cloudspannerecosystem/memefish" | ||
| "github.com/cloudspannerecosystem/memefish/ast" | ||
|
|
@@ -330,18 +331,23 @@ var clientSideStatementDefs = []*clientSideStatementDef{ | |
| Descriptions: []clientSideStatementDescription{ | ||
| { | ||
| Usage: `Start R/W transaction`, | ||
| Syntax: `BEGIN RW [TRANSACTION] [PRIORITY {HIGH|MEDIUM|LOW}]`, | ||
| Syntax: `BEGIN RW [TRANSACTION] [ISOLATION LEVEL {SERIALIZABLE|REPEATABLE READ}] [PRIORITY {HIGH|MEDIUM|LOW}]`, | ||
| Note: `(spanner-cli style); See [Request Priority](#request-priority) for details on the priority.`, | ||
| }, | ||
| }, | ||
| Pattern: regexp.MustCompile(`(?is)^BEGIN\s+RW(?:\s+TRANSACTION)?(?:\s+PRIORITY\s+(HIGH|MEDIUM|LOW))?$`), | ||
| Pattern: regexp.MustCompile(`(?is)^BEGIN\s+RW(?:\s+TRANSACTION)?(?:\s+ISOLATION\s+LEVEL\s+(SERIALIZABLE|REPEATABLE\s+READ))?(?:\s+PRIORITY\s+(HIGH|MEDIUM|LOW))?$`), | ||
| HandleSubmatch: func(matched []string) (Statement, error) { | ||
| priority, err := parsePriority(matched[1]) | ||
| isolationLevel, err := parseIsolationLevel(matched[1]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &BeginRwStatement{Priority: priority}, nil | ||
| priority, err := parsePriority(matched[2]) | ||
|
Comment on lines
339
to
+345
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you're using numeric indexing for the regex submatches, it's important to ensure that the indexes are correct. It looks like |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &BeginRwStatement{IsolationLevel: isolationLevel, Priority: priority}, nil | ||
| }, | ||
| }, | ||
| { | ||
|
|
@@ -390,14 +396,19 @@ var clientSideStatementDefs = []*clientSideStatementDef{ | |
| Note: "(Spanner JDBC driver style); It respects `READONLY` system variable. See [Request Priority](#request-priority) for details on the priority.", | ||
| }, | ||
| }, | ||
| Pattern: regexp.MustCompile(`(?is)^BEGIN(?:\s+TRANSACTION)?(?:\s+PRIORITY\s+(HIGH|MEDIUM|LOW))?$`), | ||
| Pattern: regexp.MustCompile(`(?is)^BEGIN(?:\s+TRANSACTION)?(?:\s+ISOLATION\s+LEVEL\s+(SERIALIZABLE|REPEATABLE\s+READ))?(?:\s+PRIORITY\s+(HIGH|MEDIUM|LOW))?$`), | ||
| HandleSubmatch: func(matched []string) (Statement, error) { | ||
| priority, err := parsePriority(matched[1]) | ||
| isolationLevel, err := parseIsolationLevel(matched[1]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| priority, err := parsePriority(matched[2]) | ||
|
Comment on lines
+399
to
+406
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &BeginStatement{Priority: priority}, nil | ||
| return &BeginStatement{IsolationLevel: isolationLevel, Priority: priority}, nil | ||
| }, | ||
| }, | ||
| { | ||
|
|
@@ -755,3 +766,17 @@ func exprToFullName(expr ast.Expr) (string, error) { | |
| return "", fmt.Errorf("must be ident or path, but: %T", expr) | ||
| } | ||
| } | ||
|
|
||
| func parseIsolationLevel(isolationLevel string) (sppb.TransactionOptions_IsolationLevel, error) { | ||
| if isolationLevel == "" { | ||
| return sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, nil | ||
| } | ||
|
|
||
| value := strings.Join(strings.Fields(strings.ToUpper(isolationLevel)), "_") | ||
|
|
||
| p, ok := sppb.TransactionOptions_IsolationLevel_value[value] | ||
| if !ok { | ||
| return sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, fmt.Errorf("invalid isolation level: %q", value) | ||
| } | ||
| return sppb.TransactionOptions_IsolationLevel(p), nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,7 @@ func TestRequestPriority(t *testing.T) { | |
| } | ||
|
|
||
| // Read-Write Transaction. | ||
| if err := session.BeginReadWriteTransaction(ctx, test.transactionPriority); err != nil { | ||
| if err := session.BeginReadWriteTransaction(ctx, 0, test.transactionPriority); err != nil { | ||
| t.Fatalf("failed to begin read write transaction: %v", err) | ||
| } | ||
| iter, _ := session.RunQuery(ctx, spanner.NewStatement("SELECT * FROM t1")) | ||
|
|
@@ -171,24 +171,40 @@ func TestIsolationLevel(t *testing.T) { | |
| } | ||
|
|
||
| for _, test := range []struct { | ||
| desc string | ||
| defaultIsolationLevel sppb.TransactionOptions_IsolationLevel | ||
| want sppb.TransactionOptions_IsolationLevel | ||
| desc string | ||
| defaultIsolationLevel sppb.TransactionOptions_IsolationLevel | ||
| transactionIsolationLevel sppb.TransactionOptions_IsolationLevel | ||
| want sppb.TransactionOptions_IsolationLevel | ||
| }{ | ||
| { | ||
| desc: "use default isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| want: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| desc: "use default isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| transactionIsolationLevel: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| want: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| }, | ||
| { | ||
| desc: "use serializable isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_SERIALIZABLE, | ||
| want: sppb.TransactionOptions_SERIALIZABLE, | ||
| desc: "use default serializable isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_SERIALIZABLE, | ||
| transactionIsolationLevel: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| want: sppb.TransactionOptions_SERIALIZABLE, | ||
| }, | ||
| { | ||
| desc: "use repeatable_read isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_REPEATABLE_READ, | ||
| want: sppb.TransactionOptions_REPEATABLE_READ, | ||
| desc: "use default repeatable read isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_REPEATABLE_READ, | ||
| transactionIsolationLevel: sppb.TransactionOptions_ISOLATION_LEVEL_UNSPECIFIED, | ||
| want: sppb.TransactionOptions_REPEATABLE_READ, | ||
| }, | ||
| { | ||
| desc: "use override serializable isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_REPEATABLE_READ, | ||
| transactionIsolationLevel: sppb.TransactionOptions_SERIALIZABLE, | ||
| want: sppb.TransactionOptions_SERIALIZABLE, | ||
| }, | ||
| { | ||
| desc: "use override repeatable read isolation level", | ||
| defaultIsolationLevel: sppb.TransactionOptions_SERIALIZABLE, | ||
| transactionIsolationLevel: sppb.TransactionOptions_REPEATABLE_READ, | ||
| want: sppb.TransactionOptions_REPEATABLE_READ, | ||
| }, | ||
| } { | ||
|
Comment on lines
173
to
209
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| t.Run(test.desc, func(t *testing.T) { | ||
|
|
@@ -205,18 +221,15 @@ func TestIsolationLevel(t *testing.T) { | |
| } | ||
|
|
||
| // Read-Write Transaction. | ||
| if err := session.BeginReadWriteTransaction(ctx, sppb.RequestOptions_PRIORITY_UNSPECIFIED); err != nil { | ||
| if err := session.BeginReadWriteTransaction(ctx, test.transactionIsolationLevel, sppb.RequestOptions_PRIORITY_UNSPECIFIED); err != nil { | ||
| t.Fatalf("failed to begin read write transaction: %v", err) | ||
| } | ||
| iter, _ := session.RunQuery(ctx, spanner.NewStatement("SELECT * FROM t1")) | ||
| iter, _ := session.RunQuery(ctx, spanner.NewStatement("SELECT 1")) | ||
| if err := iter.Do(func(r *spanner.Row) error { | ||
| return nil | ||
| }); err != nil { | ||
| t.Fatalf("failed to run query: %v", err) | ||
| } | ||
| if _, _, _, _, err := session.RunUpdate(ctx, spanner.NewStatement("DELETE FROM t1 WHERE Id = 1"), true); err != nil { | ||
| t.Fatalf("failed to run update: %v", err) | ||
| } | ||
| if _, err := session.CommitReadWriteTransaction(ctx); err != nil { | ||
| t.Fatalf("failed to commit: %v", err) | ||
| } | ||
|
|
||
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.
The regex
(?is)^BEGIN\s+RW(?:\s+TRANSACTION)?(?:\s+ISOLATION\s+LEVEL\s+(SERIALIZABLE|REPEATABLE\s+READ))?(?:\s+PRIORITY\s+(HIGH|MEDIUM|LOW))?$uses numeric indexing for submatches. If the order of the optional clauses changes, the indexes inHandleSubmatchwill need to be manually updated. Consider using named capture groups to improve maintainability. For example,(?is)^BEGIN\s+RW(?:\s+TRANSACTION)?(?:\s+ISOLATION\s+LEVEL\s+(?P<isolation>SERIALIZABLE|REPEATABLE\s+READ))?(?:\s+PRIORITY\s+(?P<priority>HIGH|MEDIUM|LOW))?$