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

support DEFAULT keyword #37

Merged
merged 4 commits into from Jun 18, 2022
Merged

Conversation

neglect-yp
Copy link
Contributor

close: #36

This PR does following things to add support for the DEFAULT keyword.

  • update cloud.google.com/go/spanner to v1.33.0
  • add support for the DEFAULT keyword to hammer.AlterColumn and hammer.Update
  • use a default value instead of an UPDATE statement when a NOT NULL column is added

@hhanda
Copy link

hhanda commented Jun 17, 2022

Any chances of by when this would be merged and available as a feature ? We have been using Hammer as our schema management tool, but this feature being available in Spanner and missing in Hammer is causing us to not utilize the feature.

@daichirata
Copy link
Owner

Sorry for the late reply. I had missed this PR.

@neglect-yp The test for the default value of the timestamp is giving me an error, can you fix that part?

@neglect-yp
Copy link
Contributor Author

neglect-yp commented Jun 17, 2022

Sorry. I only partially ran some tests after googleapis/google-cloud-go#6077.

I fixed the test at c5dec3a. Please rerun CI.

@neglect-yp
Copy link
Contributor Author

neglect-yp commented Jun 17, 2022

And I added some test cases to guarantee DROP DEFAULT is not generated when columns with default values are added.

@daichirata daichirata merged commit fe89490 into daichirata:master Jun 18, 2022
@daichirata
Copy link
Owner

Merged. thank you!

@hhanda
Copy link

hhanda commented Jul 29, 2022

Hi

We are getting following error:

Syntax error on line 3, column 32: Expecting ')' but found 'DEFAULT'
CREATE TABLE best_table_name (
      primary_key_attribute STRING(36) NOT NULL,
      best_boolean_attribute_name BOOL NOT NULL DEFAULT (FALSE),
      inserted_at TIMESTAMP NOT NULL,
      updated_at TIMESTAMP NOT NULL
) PRIMARY KEY(primary_key_attribute);

As per this MR hammer supports DEFAULT keyword, so confused if that' something we are doing wrong in our schema DDL.
We are using the latest hammer version hammer-0.5.9-darwin-amd64

@neglect-yp
Copy link
Contributor Author

@hhanda

I have tried to reproduce that error using the following steps, but could not reproduce it.

  1. Save your DDL as foo.sql
  2. Download hammer-0.5.9-darwin-amd64.tar.gz from https://github.com/daichirata/hammer/releases/tag/v0.5.9
  3. tar xvf ~/Downloads/hammer-0.5.9-darwin-amd64.tar.gz in ~/hammer-0.5.9
  4. ~/hammer-0.5.9/hammer-0.5.9-darwin-amd64/hammer diff <(echo) foo.sql
$ ~/hammer-0.5.9/hammer-0.5.9-darwin-amd64/hammer diff <(echo) foo.sql
CREATE TABLE best_table_name (
  primary_key_attribute STRING(36) NOT NULL,
  best_boolean_attribute_name BOOL NOT NULL DEFAULT (FALSE),
  inserted_at TIMESTAMP NOT NULL,
  updated_at TIMESTAMP NOT NULL,
) PRIMARY KEY(primary_key_attribute);

SHA-256 checksum of my hammer binary is 2ecec7b4861cc4c7a0f45e2aeaedc11110111d3aa7e04284b7a7b1b94ee27900. Please check that your binary has a same checksum.

$ sha256sum ~/hammer-0.5.9/hammer-0.5.9-darwin-amd64/hammer | awk '{print $1}'
2ecec7b4861cc4c7a0f45e2aeaedc11110111d3aa7e04284b7a7b1b94ee27900

@hhanda
Copy link

hhanda commented Aug 2, 2022

Thanks @daichirata for replying so quickly, was just checking and it seems that error is not from the sql that I am trying to apply but rather from the parsing that hammer is trying to do from what' already in the spanner DB.

it' basically failing on while parsing the below line in our schema:

expiry_at TIMESTAMP AS (IF(another_column_name IS NULL AND status='open', updated_at, '9000-01-01 00:00:00 UTC')) STORED,

@neglect-yp
Copy link
Contributor Author

It seems to be the same issue with #38.

@hhanda
Copy link

hhanda commented Aug 11, 2022

@neglect-yp yeah but that subsequent MR referred in the above MR is already merged, so this means it would still be sometime before these conditions will be supported correct ?

@neglect-yp
Copy link
Contributor Author

@hhanda
You can try hammer v0.5.10, which has been released with added support for IF and IFNULL expressions. It will work fine if you are not using NULLIF or COALESCE expressions (or other unsupported syntax).

I have not checked your entire DDL, so I do not know if support for only IF and IFNULL expressions is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support DEFAULT keyword
3 participants