*: fix the `origin_default_value` issue in disaggregated mode by zimulala · Pull Request #9666 · pingcap/tiflash

zimulala · 2024-11-22T03:14:52Z

What problem does this PR solve?

Issue Number: close #9665

Problem Summary:

What is changed and how it works?

Since the TiDB add new column operation does not actually write the new column data, it simply logs it to `origin_default_value`.
In disaggregated mode, the default value may be read using tipb passing the default value. The original logic is to pass the default value of tipb to the default value of ColumnInfo (calling defaultValueToField).

After obtaining the default value of tipb, decode it, and then use the defaultValueToField value for processing.

Check List

Tests

Unit test
Integration test

tidb> set session tidb_isolation_read_engines='tiflash';
tidb> select col1, col2, col3, col4, col5, col6, col7, col8, col9, col10, col11, col12, col13, col14, hex(col15) from test.t;

Before this PR:

+------+------+------+------+------+-------+------+------+------+-------+-------+-------+----------+----------+-------+
| col1 | col2 | col3 | col4 | col5 | col6  | col7 | col8 | col9 | col10 | col11 | col12 | col13    | col14    |col15 |
+------+------+------+------+------+-------+------+------+------+-------+-------+-------+----------+----------+-------+
|    0 | 0000 | 0.00 | 0.00 |    0 | sss | NULL | NULL | 0000 |       |       | NULL  | 00:00:00 | 00:00:00 | NULL  |
|    0 | 0000 | 0.00 | 0.00 |    0 | sss | NULL | NULL | 0000 |       |       | NULL  | 00:00:00 | 00:00:00 | NULL  |
|    0 | 0000 | 0.00 | 0.00 |    0 | sss | NULL | NULL | 0000 |       |       | NULL  | 00:00:00 | 00:00:00 | NULL  |
+------+------+------+------+------+-------+------+------+------+-------+-------+-------+----------+----------+-------+

After this PR:

+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+---------------------+----------+-------+------------+
| col1 | col2 | col3 | col4 | col5 | col6 | col7                | col8                | col9 | col10  | col11  | col12               | col13    | col14 | hex(col15) |
+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+---------------------+----------+-------+------------+
|   30 | 0000 | 0.00 | 1.23 |  123 | sss  | 2017-02-11 00:00:00 | 2018-11-04 23:12:03 | 2012 | value3 | value1 | 2018-11-04 23:12:03 | 12:18:30 | NULL  | 10203      |
|   30 | 0000 | 0.00 | 1.23 |  123 | sss  | 2017-02-11 00:00:00 | 2018-11-04 23:12:03 | 2012 | value3 | value1 | 2018-11-04 23:12:03 | 12:18:30 | NULL  | 10203      |
|   30 | 0000 | 0.00 | 1.23 |  123 | sss  | 2017-02-11 00:00:00 | 2018-11-04 23:12:03 | 2012 | value3 | value1 | 2018-11-04 23:12:03 | 12:18:30 | NULL  | 10203      |
+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+---------------------+----------+-------+------------+

Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix the bug that in the disaggregated storage and compute architecture, the query result of added column is not correct

sre-bot · 2024-11-22T03:14:59Z

All committers have signed the CLA.

zimulala · 2024-11-22T04:54:16Z

PTAL @Lloyd-Pottiger

dbms/src/TiDB/Schema/TiDB.cpp

Lloyd-Pottiger · 2024-11-22T05:51:46Z

dbms/src/TiDB/Schema/TiDB.cpp

+            if (val.get<Int64>() == 0)
+            {
+                Poco::Dynamic::Var empty_val;
+                tidb_column_info.origin_default_value = empty_val;
+                break;
+            }


set tidb_column_info.origin_default_value = tipb_column_info.default_val(); if val.get<Int64>() != 0 ?

Yes. If we don't do this, after alter table t add column col2 year not null;, we get this data is 2000 not 0000.

dbms/src/TiDB/Schema/TiDB.cpp

zimulala · 2024-11-22T08:02:42Z

PTAL @Lloyd-Pottiger

Lloyd-Pottiger

LGTM

dbms/src/TiDB/Schema/TiDB.cpp

tests/fullstack-test2/ddl/alter_default_value.test

JaySon-Huang

LGTM

ti-chi-bot · 2024-11-25T09:36:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-11-25T09:36:35Z

[LGTM Timeline notifier]

Timeline:

2024-11-22 08:10:21.654749461 +0000 UTC m=+192009.274403977: ☑️ agreed by Lloyd-Pottiger.
2024-11-25 09:36:34.430402402 +0000 UTC m=+456382.050056918: ☑️ agreed by JaySon-Huang.

ti-chi-bot · 2024-11-25T10:40:49Z

@zimulala: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2024-11-28T13:25:59Z

In response to a cherrypick label: new pull request created to branch release-7.5: #9683.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

…#9683) close #9665 Since the TiDB add new column operation does not actually write the new column data, it simply logs it to `origin_default_value`. In disaggregated mode, the default value may be read using tipb passing the default value. The original logic is to pass the default value of tipb to the default value of ColumnInfo (calling defaultValueToField). After obtaining the default value of tipb, decode it, and then use the defaultValueToField value for processing. Signed-off-by: JaySon-Huang <tshent@qq.com> Co-authored-by: JaySon-Huang <tshent@qq.com> Co-authored-by: JaySon <tshent@qq.com>

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2024-12-11T07:56:58Z

In response to a cherrypick label: new pull request created to branch release-8.1: #9719.

…#9719) close #9665 Since the TiDB add new column operation does not actually write the new column data, it simply logs it to `origin_default_value`. In disaggregated mode, the default value may be read using tipb passing the default value. The original logic is to pass the default value of tipb to the default value of ColumnInfo (calling defaultValueToField). After obtaining the default value of tipb, decode it, and then use the defaultValueToField value for processing. Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: JaySon-Huang <tshent@qq.com> Co-authored-by: Lynn <zimu_xia@126.com> Co-authored-by: JaySon-Huang <tshent@qq.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot · 2024-12-13T09:34:31Z

In response to a cherrypick label: new pull request created to branch release-8.5: #9729.

…#9729) close #9665 Since the TiDB add new column operation does not actually write the new column data, it simply logs it to `origin_default_value`. In disaggregated mode, the default value may be read using tipb passing the default value. The original logic is to pass the default value of tipb to the default value of ColumnInfo (calling defaultValueToField). After obtaining the default value of tipb, decode it, and then use the defaultValueToField value for processing. Co-authored-by: Lynn <zimu_xia@126.com> Co-authored-by: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com>

*: fix the original default value issue

a7fc1a0

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 22, 2024

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 22, 2024

zimulala changed the title ~~[DNM]*: fix the origin_default_value issue in disaggregated mode~~ *: fix the origin_default_value issue in disaggregated mode Nov 22, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Nov 22, 2024

Lloyd-Pottiger reviewed Nov 22, 2024

View reviewed changes

*: tiny update

e641193

Lloyd-Pottiger reviewed Nov 22, 2024

View reviewed changes

dbms/src/TiDB/Schema/TiDB.cpp Outdated Show resolved Hide resolved

*: address comments

21a69e8

Lloyd-Pottiger approved these changes Nov 22, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 22, 2024

Lloyd-Pottiger requested review from JaySon-Huang and JinheLin November 22, 2024 08:10

*: add a comment

a10ed18

JaySon-Huang reviewed Nov 25, 2024

View reviewed changes

dbms/src/TiDB/Schema/TiDB.cpp Outdated Show resolved Hide resolved

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 25, 2024

*: address a comment

14f21f1

JaySon-Huang reviewed Nov 25, 2024

View reviewed changes

tests/fullstack-test2/ddl/alter_default_value.test Outdated Show resolved Hide resolved

JaySon-Huang approved these changes Nov 25, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 25, 2024

*: tiny update a test

a81900b

zimulala force-pushed the zimuxia/add-col-default branch from 0c22cb5 to a81900b Compare November 25, 2024 10:27

Merge branch 'master' into zimuxia/add-col-default

8027c55

ti-chi-bot bot merged commit e3e6780 into pingcap:master Nov 25, 2024

zimulala deleted the zimuxia/add-col-default branch November 26, 2024 02:23

ti-chi-bot bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Nov 28, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Nov 28, 2024

This is an automated cherry-pick of pingcap#9666

c2ffb21

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Nov 28, 2024

*: fix the origin_default_value issue in disaggregated mode (#9666) #9683

Merged

12 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label Dec 11, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Dec 11, 2024

This is an automated cherry-pick of pingcap#9666

5cb3c4c

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Dec 11, 2024

*: fix the origin_default_value issue in disaggregated mode (#9666) #9719

Merged

12 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Dec 13, 2024

ti-chi-bot mentioned this pull request Dec 13, 2024

*: fix the origin_default_value issue in disaggregated mode (#9666) #9729

Merged

12 tasks

Conversation

zimulala commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Before this PR:

After this PR:

Release note

Uh oh!

sre-bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimulala commented Nov 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lloyd-Pottiger Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

zimulala Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zimulala commented Nov 22, 2024

Uh oh!

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JaySon-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Nov 25, 2024

Uh oh!

ti-chi-bot bot commented Nov 25, 2024

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Nov 25, 2024

Uh oh!

ti-chi-bot commented Nov 28, 2024

Uh oh!

ti-chi-bot commented Dec 11, 2024

Uh oh!

ti-chi-bot commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zimulala commented Nov 22, 2024 •

edited

Loading

sre-bot commented Nov 22, 2024 •

edited

Loading