daichirata / hammer

🛠 hammer is a command-line tool to schema management for Google Cloud Spanner.
MIT License
84 stars 25 forks source link

support DEFAULT keyword #37

Closed neglect-yp closed 2 years ago

neglect-yp commented 2 years ago

close: https://github.com/daichirata/hammer/issues/36

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

hhanda commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Sorry. I only partially ran some tests after https://github.com/googleapis/google-cloud-go/pull/6077.

I fixed the test at c5dec3a. Please rerun CI.

neglect-yp commented 2 years ago

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

daichirata commented 2 years ago

Merged. thank you!

hhanda commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

It seems to be the same issue with https://github.com/daichirata/hammer/issues/38.

hhanda commented 2 years ago

@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 commented 2 years ago

@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.