feat(tianmu): support add integer/decimal column DDL with default value. (#1187) #1208

lujiashun · 2023-01-10T13:12:26Z

Summary about this PR

Issue Number: close #1187

[summary]
check the default value of field,please see tianmu_attr.cpp for details:

if field is integer type but not real type;
if field is integer type and also real type;
if field is not integer type;

Tests Check List

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

Changelog

Documentation

Affects user behaviors
Contains syntax changes
Contains variable changes
Contains experimental features

mergify · 2023-01-10T13:13:00Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

wisehead · 2023-01-11T13:58:02Z

storage/tianmu/core/tianmu_attr.cpp

+  size_t no_nulls{no_rows};
+  int64_t min{0};
+  int64_t max{0};
+  bool int_not_null{false};


why not use AttributeTypeInfo::NotNull() to check whether the field is NULL? I guess int_not_null means has_default_value?

1.not null flag check is also a solution. in the scenario the alter table defined not null or null(same with tianmu), the
output is NULL. I just want to use GetValueFromField(it deals the default value logic) function to deal with the scenario, the std::variants store the value, it has the interface to decide whether it is null,if not null, what is the default value.
2 The current solution is also a proper way,not to consider the null flag (or it has been considered in GetValueFromField)
3 the std::variant in gcc8 has bugs, what we supported is gcc9.3, if we do not upgrade debian gcc to 9.3, another solution is to use boost:variant, but not recommended. It is better to upgrade the debian CI to gcc9.3 @wisehead

wisehead · 2023-01-11T13:59:03Z

storage/tianmu/handler/ha_tianmu.cpp

@@ -112,7 +112,7 @@ static core::Value GetValueFromField(Field *f) {
      // convert to UTC
      if (!common::IsTimeStampZero(my_time)) {
        my_bool myb;
-        my_time_t secs_utc = current_txn_->Thd()->variables.time_zone->TIME_to_gmt_sec(&my_time, &myb);
+        my_time_t secs_utc = f->table->in_use->variables.time_zone->TIME_to_gmt_sec(&my_time, &myb);


please add some comments here, I don't quite understand the code changes here.

f->table->in_use is to get the THD pionter, current_txn_->Thd() is also to get the THD pointer. I want to store default value from the function GetValueFromField, but in some senario, the cureent_txn is not initialized and pointed to null. I find f->table->in_use is not nullptr, so i change the code to f->table->in_use.

wisehead · 2023-01-11T14:00:25Z

storage/tianmu/handler/ha_tianmu.cpp

@@ -1634,7 +1634,13 @@ bool ha_tianmu::inplace_alter_table(TABLE *altered_table, Alter_inplace_info *ha
        DBUG_RETURN(false);
    } else if (!(ha_alter_info->handler_flags & ~TIANMU_SUPPORTED_ALTER_ADD_DROP_ORDER)) {
      std::vector<Field *> v_old(table_share->field, table_share->field + table_share->fields);
-      std::vector<Field *> v_new(altered_table->s->field, altered_table->s->field + altered_table->s->fields);
+
+      Field_iterator_table it_field;


please also add some comments here, does original code work? or you change it because it's just ugly?

Because i want to get the default value from the filed, but altered_table->s->field's table pointer is null, can not to get the default value. alter_table->field's table pointer is correct, so i use it to store the fields;

wisehead · 2023-01-11T14:04:47Z

@DandreChen @hustjieke please also review the code.

codecov · 2023-01-12T03:35:51Z

Codecov Report

Base: 43.38% // Head: 43.33% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (5ed4b4b) compared to base (e65c757).
Patch coverage: 69.23% of modified lines in pull request are covered.

❗ Current head 5ed4b4b differs from pull request most recent head 82e8dca. Consider uploading reports for the commit 82e8dca to get more accurate results

Additional details and impacted files

@@                 Coverage Diff                 @@
##           stonedb-5.7-dev    #1208      +/-   ##
===================================================
- Coverage            43.38%   43.33%   -0.05%     
===================================================
  Files                 1830     1830              
  Lines               396143   396147       +4     
===================================================
- Hits                171853   171677     -176     
- Misses              224290   224470     +180

Impacted Files	Coverage Δ
storage/tianmu/core/engine.cpp	`53.46% <0.00%> (-1.21%)`	⬇️
storage/tianmu/core/engine.h	`30.43% <ø> (ø)`
storage/tianmu/core/tianmu_attr_typeinfo.h	`81.25% <ø> (+1.25%)`	⬆️
storage/tianmu/handler/ha_tianmu.h	`87.50% <ø> (ø)`
storage/tianmu/core/tianmu_attr.cpp	`71.74% <73.52%> (+0.05%)`	⬆️
storage/tianmu/core/value.h	`88.88% <85.71%> (-11.12%)`	⬇️
storage/tianmu/core/tianmu_attr_typeinfo.cpp	`60.00% <100.00%> (+10.00%)`	⬆️
storage/tianmu/handler/ha_tianmu.cpp	`57.33% <100.00%> (+0.34%)`	⬆️
storage/tianmu/core/multi_index_builder.h	`20.00% <0.00%> (-60.00%)`	⬇️
storage/tianmu/common/common_definitions.cpp	`57.14% <0.00%> (-28.11%)`	⬇️
... and 60 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hustjieke · 2023-01-12T11:09:48Z

ACK.

RingsC · 2023-01-31T08:52:02Z

mysql-test/suite/tianmu/t/issue1187.test

+
+create table t3(id int,name varchar(5)) ;
+insert into t3 values(1,'AAA'),(2,'BBB');
+alter table t3 add column age int not null default 66;


Pls, add some case for errors. such as using wrong default value.
Taking the following as an instance.
The column type is char but using integer as a default value, the type and default value is mismatch. or the default will lead to type overflow.

F.I.Y: Just only change the default value.
ALTER TABLE table_name ALTER COLUMN column_name SET DEFAULT 'literal';

ok, i will try to add some wrong default value; maybe it will be intercepted by sql layer, i will test it.(confirmed, it wll be intercepted by sql layer, better not to add this mtr);

"Just only change the default value" is a related sceanrio, i will add this MTR.

storage/tianmu/handler/ha_tianmu.cpp

RingsC · 2023-01-31T08:57:36Z

storage/tianmu/handler/ha_tianmu.h

@@ -23,6 +23,7 @@

 namespace Tianmu {
 namespace handler {
+extern Tianmu::core::Value GetValueFromField(Field *f);


Or perhaps, put it in .cpp, in which we will use this function, is a better way. And, will not contaminate the other files which includes this .h file.

OK, i will fix it;

storage/tianmu/core/value.h

storage/tianmu/core/engine.cpp

…ue. (stoneatom#1187) [summary] check the default value of field,please see tianmu_attr.cpp for details: 1 if field is integer type but not real type; 2 if field is integer type and also real type; 2if field is not integer type;

DandreChen · 2023-01-31T13:43:39Z

mysql-test/suite/tianmu/t/issue1187.test

+
+create table t2(id int,name varchar(5)) ;
+insert into t2 values(1,'AAA'),(2,'BBB');
+alter table t2 add column age int not null;


when the type is varchar not null，what will happen？

if it is convenient，please fix it together when the type is varchar not null
as below case:

create table ttt(id int,name varchar(5)); insert into ttt values(1,'AAA'),(2,'BBB'); alter table ttt add column age varchar(5) not null; select * from ttt;

lujiashun · 2023-02-01T11:02:57Z

The current inplace ddl solution(include the original) seems that it does not consider the delete row scenario, need a deeper check.

lujiashun · 2023-02-07T08:38:52Z

default NULL
1.1 default NULL, not exist delete rows; [dpn]
1.2 default NULL, exist delete rows; [dpn + null part + delete part]
not default NULL
2.1 not default NULL, not exist delete rows, interger type;[dpn]
2.2 not default NULL, exist delete rows, interger type;[dpn + null part + delete part]
2.3 not defualt NULL, string type, [dpn + null part + delete part + data part]

lujiashun · 2023-02-08T01:58:31Z

Alter Table Add Column Design(High Level)

1 current status
only suppport the scenario that default value is NULL and rows are not ever deleted;
not support the scenario that default value is NULL and row are ever deleted;
not support the scenario that default value is not NULL;

1.1 scenario: default value is NULL and rows are not ever deleted[supported]
only require to create DPN file, not need to create DATA file;

1.2 scenario: default value is NULL and rows are ever deleted;
Because of the deletion of rows, including creating DPNfile, DATA file is need;

1.3 scenario:default value is not NULL
it can be devided into three sub-scenarios
1.3.1 default not null, rows are not deleted, data type is interger(DPN)
1.3.2 default not null, row are ever deleted, data type is interger(DPN + DATA)
1.3.3 default not null, data type is string(DPN + DATA)

2 How to get and create data file
2.1 get old columns by tianmu_table::m_attrs,Only the first column is required;
2.2 get the related pack indx by tianmu_attr::m_ids;
2.3 lock the related pack by tianmu_attr::LockPackForUse;
2.4 parallely read the pack/pack node through by tianmu_attr::get_pack/get_dpn;
2.5 parallely construct the new pack from old pack by pack::clone or refer to copybackforwrite.
2.6 need to add a new pack constructor because new pack need a new path; Refer code like this:

PackStr::PackStr(const PackStr &aps, const PackCoordinate &pc) : Pack(aps, pc) {
    try {
        data_.len_mode = aps.data_.len_mode;
        data_.lens = alloc((data_.len_mode * (1 << col_share_->pss)), mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED);
        std::memset(data_.lens, 0, data_.len_mode * (1 << col_share_->pss));
        data_.index =
            reinterpret_cast<char **>(alloc(sizeof(char *) * (1 << col_share_->pss), mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED));

        data_.sum_len = aps.data_.sum_len;
        data_.v.push_back(
            {reinterpret_cast<char *>(alloc(data_.sum_len + 1, mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED)), data_.sum_len, 0});

        for (uint i = 0; i < aps.dpn_->numOfRecords; i++) {
            if (aps.IsNull(i)) {
                SetPtr(i, nullptr);
                continue;
            }
            auto value = aps.GetValueBinary(i);
            auto size = value.size();
            if (size != 0) {
                SetPtrSize(i, Put(value.GetDataBytesPointer(), size), size);
            } else {
                SetPtrSize(i, nullptr, 0);
            }
        }
    } catch (...) {
        Destroy();
        throw;
    }
}

2.7 update version file of attr;
2.8 update version file of table;

3 How to get default value
3.1 default value is can be stored in the datatype Tianmu::core::value by GetValueFromField;

4 Test for performance
4.1 use big data to compare the speed betwen copy mode and inplace mode(this design);

lujiashun · 2023-02-08T01:59:43Z

Alter Table Add Column Design(High Level)

1 current status only suppport the scenario that default value is NULL and rows are not ever deleted; not support the scenario that default value is NULL and row are ever deleted; not support the scenario that default value is not NULL;

1.1 scenario: default value is NULL and rows are not ever deleted[supported] only require to create DPN file, not need to create DATA file;

1.2 scenario: default value is NULL and rows are ever deleted; Because of the deletion of rows, including creating DPNfile, DATA file is need;

1.3 scenario:default value is not NULL it can be devided into three sub-scenarios 1.3.1 default not null, rows are not deleted, data type is interger(DPN) 1.3.2 default not null, row are ever deleted, data type is interger(DPN + DATA) 1.3.3 default not null, data type is string(DPN + DATA)

2 How to get and create data file 2.1 get old columns by tianmu_table::m_attrs,Only the first column is required; 2.2 get the related pack indx by tianmu_attr::m_ids; 2.3 lock the related pack by tianmu_attr::LockPackForUse; 2.4 parallely read the pack/pack node through by tianmu_attr::get_pack/get_dpn; 2.5 parallely construct the new pack from old pack by pack::clone or refer to copybackforwrite. 2.6 need to add a new pack constructor because new pack need a new path; Refer code like this:
PackStr::PackStr(const PackStr &aps, const PackCoordinate &pc) : Pack(aps, pc) {
    try {
        data_.len_mode = aps.data_.len_mode;
        data_.lens = alloc((data_.len_mode * (1 << col_share_->pss)), mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED);
        std::memset(data_.lens, 0, data_.len_mode * (1 << col_share_->pss));
        data_.index =
            reinterpret_cast<char **>(alloc(sizeof(char *) * (1 << col_share_->pss), mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED));

        data_.sum_len = aps.data_.sum_len;
        data_.v.push_back(
            {reinterpret_cast<char *>(alloc(data_.sum_len + 1, mm::BLOCK_TYPE::BLOCK_UNCOMPRESSED)), data_.sum_len, 0});

        for (uint i = 0; i < aps.dpn_->numOfRecords; i++) {
            if (aps.IsNull(i)) {
                SetPtr(i, nullptr);
                continue;
            }
            auto value = aps.GetValueBinary(i);
            auto size = value.size();
            if (size != 0) {
                SetPtrSize(i, Put(value.GetDataBytesPointer(), size), size);
            } else {
                SetPtrSize(i, nullptr, 0);
            }
        }
    } catch (...) {
        Destroy();
        throw;
    }
}
2.7 update version file of attr; 2.8 update version file of table;

3 How to get default value 3.1 default value is can be stored in the datatype Tianmu::core::value by GetValueFromField;

4 Test for performance 4.1 use big data to compare the speed betwen copy mode and inplace mode(this design);

@wisehead @RingsC @hustjieke @DandreChen

CLAassistant · 2023-04-13T07:10:17Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

lujiashun seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

lujiashun added the PR-feature feature for pull request label Jan 10, 2023

lujiashun requested review from RingsC, wisehead, hustjieke, DandreChen, isredstar, haitaoguan and konghaiya January 10, 2023 13:12

lujiashun self-assigned this Jan 10, 2023

lujiashun marked this pull request as draft January 10, 2023 13:12

lujiashun added this to the stonedb_5.7_v1.0.3 milestone Jan 10, 2023

lujiashun force-pushed the feat-1187-stonedb5.7 branch from 9f2bdbb to 4146fa6 Compare January 11, 2023 02:29

lujiashun changed the title ~~feat(tianmu): fix bug: master/slave, after an integer column is added,InnoDB defaults to 0, but tianmu defaults to null. (#1187)~~ feat(tianmu): support add integer/decimal column DDL with default value. (#1187) Jan 11, 2023

lujiashun force-pushed the feat-1187-stonedb5.7 branch 2 times, most recently from 5d681ba to 13f62b6 Compare January 11, 2023 06:08

wisehead reviewed Jan 11, 2023

View reviewed changes

lujiashun force-pushed the feat-1187-stonedb5.7 branch 2 times, most recently from d2eb312 to 62cd214 Compare January 12, 2023 03:12

lujiashun marked this pull request as ready for review January 12, 2023 03:35

lujiashun force-pushed the feat-1187-stonedb5.7 branch from 62cd214 to 5ed4b4b Compare January 12, 2023 03:37

lujiashun marked this pull request as draft January 12, 2023 04:04

wisehead approved these changes Jan 12, 2023

View reviewed changes

lujiashun marked this pull request as ready for review January 13, 2023 03:47

lujiashun mentioned this pull request Jan 17, 2023

fix(tianmu): fix alter table add column not null.(#1188) #1235

Merged

14 tasks

hustjieke added the B-storage data type, data storage, insert,update,delete, transactions label Jan 30, 2023

RingsC reviewed Jan 31, 2023

View reviewed changes

lujiashun force-pushed the feat-1187-stonedb5.7 branch from 5ed4b4b to 82e8dca Compare January 31, 2023 13:26

DandreChen reviewed Jan 31, 2023

View reviewed changes

lujiashun marked this pull request as draft February 1, 2023 11:01

hustjieke modified the milestones: stonedb_5.7_v1.0.3, stonedb_5.7_v1.0.4 Mar 18, 2023

hustjieke closed this Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tianmu): support add integer/decimal column DDL with default value. (#1187) #1208

feat(tianmu): support add integer/decimal column DDL with default value. (#1187) #1208

lujiashun commented Jan 10, 2023 •

edited

Loading

mergify bot commented Jan 10, 2023

wisehead Jan 11, 2023

lujiashun Jan 11, 2023 •

edited

Loading

wisehead Jan 11, 2023

lujiashun Jan 11, 2023

wisehead Jan 11, 2023

lujiashun Jan 11, 2023

wisehead commented Jan 11, 2023

codecov bot commented Jan 12, 2023 •

edited

Loading

hustjieke commented Jan 12, 2023

RingsC Jan 31, 2023

RingsC Jan 31, 2023

lujiashun Jan 31, 2023 •

edited

Loading

RingsC Jan 31, 2023

lujiashun Jan 31, 2023 •

edited

Loading

DandreChen Jan 31, 2023 •

edited

Loading

DandreChen Jan 31, 2023

lujiashun commented Feb 1, 2023

lujiashun commented Feb 7, 2023

lujiashun commented Feb 8, 2023

lujiashun commented Feb 8, 2023

CLAassistant commented Apr 13, 2023

feat(tianmu): support add integer/decimal column DDL with default value. (#1187) #1208

feat(tianmu): support add integer/decimal column DDL with default value. (#1187) #1208

Conversation

lujiashun commented Jan 10, 2023 • edited Loading

Summary about this PR

Tests Check List

Changelog

Documentation

mergify bot commented Jan 10, 2023

Choose a reason for hiding this comment

lujiashun Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisehead commented Jan 11, 2023

codecov bot commented Jan 12, 2023 • edited Loading

Codecov Report

hustjieke commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiashun Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiashun Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

DandreChen Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lujiashun commented Feb 1, 2023

lujiashun commented Feb 7, 2023

lujiashun commented Feb 8, 2023

lujiashun commented Feb 8, 2023

CLAassistant commented Apr 13, 2023

lujiashun commented Jan 10, 2023 •

edited

Loading

lujiashun Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 12, 2023 •

edited

Loading

lujiashun Jan 31, 2023 •

edited

Loading

lujiashun Jan 31, 2023 •

edited

Loading

DandreChen Jan 31, 2023 •

edited

Loading