Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

kv: fix handling of column default values #170

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Apr 23, 2019

What problem does this PR solve?

Properly fill in the default value for a missing column.

What is changed and how it works?

  • if the column is AUTO_INCREMENT, fill in with row_id (assume it is missing for the entire table instead of just a few values)
  • if the column has DEFAULT, fill in that value
  • (if the column is NULL, fill in NULL.)

Check List

Tests

  • Integration test

Side effects

Related changes

  • Need to cherry-pick to the release branch

* if the column is AUTO_INCREMENT, fill in with row_id (assume it is
  missing for the entire table instead of just a few values)
* if the column has DEFAULT, fill in that value
@kennytm kennytm added status/PTAL This PR is ready for review. Add this label back after committing new changes priority/normal type/bug-fix Bug fix labels Apr 23, 2019
@kennytm
Copy link
Collaborator Author

kennytm commented Apr 23, 2019

/run-all-tests

@IANTHEREAL
Copy link
Collaborator

LGTM

@IANTHEREAL IANTHEREAL added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 24, 2019
} else {
value, err = table.GetColOriginDefaultValue(kvcodec.se, col.ToInfo())
value, err = table.GetColDefaultValue(kvcodec.se, col.ToInfo())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not handle some cases like current_timestamp anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CURRENT_TIMESTAMP does work, it currently fetches the actual current time 🤔. (Before this PR it was always NULL.) Gonna change to a fixed timestamp stored in the checkpoint to ensure determinism in a future PR.

@kennytm
Copy link
Collaborator Author

kennytm commented Apr 25, 2019

/run-all-tests tidb=master tikv=master pd=master

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Apr 25, 2019
@kennytm kennytm merged commit 9adb660 into master Apr 25, 2019
@kennytm kennytm deleted the kennytm/auto-inc-defaults branch April 25, 2019 03:23
@kennytm kennytm added the Should Cherry-pick This PR should be cherry-picked back the previous release train label Apr 27, 2019
kennytm added a commit that referenced this pull request Apr 27, 2019
* kv: fix handling of column default values

* if the column is AUTO_INCREMENT, fill in with row_id (assume it is
  missing for the entire table instead of just a few values)
* if the column has DEFAULT, fill in that value

* tests: ensure DEFAULT CURRENT_TIMESTAMP works
@kennytm kennytm removed the Should Cherry-pick This PR should be cherry-picked back the previous release train label Apr 27, 2019
IANTHEREAL pushed a commit that referenced this pull request Apr 28, 2019
* kv: fix handling of column default values (#170)

* kv: fix handling of column default values

* if the column is AUTO_INCREMENT, fill in with row_id (assume it is
  missing for the entire table instead of just a few values)
* if the column has DEFAULT, fill in that value

* tests: ensure DEFAULT CURRENT_TIMESTAMP works

* tests,restore: re-enable the exotic_filenames test (#172)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants