Skip to content
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

spansql: AlterColumn produces invalid SQL when there is an AllowCommitTimestamp OPTION #2621

Closed
oceanful opened this issue Jul 21, 2020 · 2 comments · Fixed by #2656
Closed
Assignees
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@oceanful
Copy link

Code

package main

import (
  "fmt"

  "cloud.google.com/go/spanner/spansql"
)

func main() {
  allow := true
  stmt := spansql.AlterTable{
    Name: "Tasks",
    Alteration: spansql.AlterColumn{
      Def: spansql.ColumnDef{
        Name: "LastUpdate",
        Type: spansql.Type{Base: spansql.Timestamp},
        AllowCommitTimestamp: &allow,
      },
    },
  }
  fmt.Println(stmt.SQL())
}

Expected behavior

AlterColumn should only alter the column type:

ALTER TABLE Tasks ALTER COLUMN LatestUpdate TIMESTAMP

To support setting options, there should probably be a separate type (maybe something like AlterOptions) that produces:

ALTER TABLE Tasks ALTER COLUMN LatestUpdate SET OPTIONS (allow_commit_timestamp = true)

Actual behavior

The resulting statement is invalid and rejected by spanner:

ALTER TABLE Tasks ALTER COLUMN LatestUpdate TIMESTAMP OPTIONS (allow_commit_timestamp = true);
Error: rpc error: code = InvalidArgument desc = Error parsing Spanner DDL statement: 
ALTER TABLE Tasks ALTER COLUMN LatestUpdateTime TIMESTAMP OPTIONS (allow_commit_timestamp = true) : 
Syntax error on line 1, column ##: Expecting 'EOF' but found 'OPTIONS'
@oceanful oceanful added the triage me I really want to be triaged. label Jul 21, 2020
@codyoss codyoss added the api: spanner Issues related to the Spanner API. label Jul 21, 2020
@larkee larkee added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Jul 22, 2020
@hengfengli hengfengli assigned dsymonds and unassigned skuruppu Jul 28, 2020
@dsymonds
Copy link
Contributor

The relevant part of https://cloud.google.com/spanner/docs/data-definition-language#ddl_syntax is:

    ALTER [ COLUMN ] column_name { { data_type } [ NOT NULL ] | SET [ options_def ] }

so when we are altering a column, either the data type (or its null-ness) may be changed, or options may be set. Looks like spansql.AlterColumn is too simplistic.

dsymonds added a commit that referenced this issue Jul 30, 2020
)

Column options appear in a few different places:
	CREATE TABLE
        ALTER TABLE ... ADD COLUMN
        ALTER TABLE ... ALTER COLUMN

In the first two places, the entire column is being defined, and the
options may appear without a leading "SET". The third place may
explicitly set options using "SET". In all cases, the options
specification is itself the same, so extract that into its own type,
ColumnOptions, and add distinguished ColumnAlteration types so the
`ALTER TABLE ... ALTER COLUMN` form may be correctly parsed and
formatted.

This change is backward incompatible, but should only affect those using
(defining or parsing) column options.

Fixes #2621.
@oceanful
Copy link
Author

Thank you!!

tritone pushed a commit to tritone/google-cloud-go that referenced this issue Aug 25, 2020
…ogleapis#2656)

Column options appear in a few different places:
	CREATE TABLE
        ALTER TABLE ... ADD COLUMN
        ALTER TABLE ... ALTER COLUMN

In the first two places, the entire column is being defined, and the
options may appear without a leading "SET". The third place may
explicitly set options using "SET". In all cases, the options
specification is itself the same, so extract that into its own type,
ColumnOptions, and add distinguished ColumnAlteration types so the
`ALTER TABLE ... ALTER COLUMN` form may be correctly parsed and
formatted.

This change is backward incompatible, but should only affect those using
(defining or parsing) column options.

Fixes googleapis#2621.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants