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

*: fix the origin_default_value issue in disaggregated mode #9666

Merged
merged 7 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions dbms/src/Common/FieldVisitors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@

namespace DB
{
template <typename T>
static inline String format(const DecimalField<T> & x)
{
WriteBufferFromOwnString wb;
writeText(x.getValue(), x.getScale(), wb);
return wb.str();
}

template <typename T>
static inline String formatQuoted(T x)
{
Expand Down Expand Up @@ -170,19 +178,27 @@ String FieldVisitorToString::operator()(const String & x) const
}
String FieldVisitorToString::operator()(const DecimalField<Decimal32> & x) const
{
return formatQuoted(x);
if (isDecimalWithQuoted)
return formatQuoted(x);
return format(x);
}
String FieldVisitorToString::operator()(const DecimalField<Decimal64> & x) const
{
return formatQuoted(x);
if (isDecimalWithQuoted)
return formatQuoted(x);
return format(x);
}
String FieldVisitorToString::operator()(const DecimalField<Decimal128> & x) const
{
return formatQuoted(x);
if (isDecimalWithQuoted)
return formatQuoted(x);
return format(x);
}
String FieldVisitorToString::operator()(const DecimalField<Decimal256> & x) const
{
return formatQuoted(x);
if (isDecimalWithQuoted)
return formatQuoted(x);
return format(x);
}

String FieldVisitorToString::operator()(const Array & x) const
Expand Down
9 changes: 9 additions & 0 deletions dbms/src/Common/FieldVisitors.h
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,16 @@ typename std::decay_t<Visitor>::ResultType applyVisitor(Visitor && visitor, F1 &
/** Prints Field as literal in SQL query */
class FieldVisitorToString : public StaticVisitor<String>
{
private:
bool isDecimalWithQuoted;

public:
FieldVisitorToString()
: isDecimalWithQuoted(true){};

FieldVisitorToString(bool val)
: isDecimalWithQuoted(val){};

String operator()(const Null & x) const;
String operator()(const UInt64 & x) const;
String operator()(const Int64 & x) const;
Expand Down
98 changes: 87 additions & 11 deletions dbms/src/TiDB/Schema/TiDB.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
#include <Common/MyTime.h>
#include <Core/Types.h>
#include <DataTypes/DataTypeDecimal.h>
#include <DataTypes/FieldToDataType.h>
#include <IO/Buffer/ReadBufferFromString.h>
#include <Poco/Base64Decoder.h>
#include <Poco/Dynamic/Var.h>
#include <Poco/MemoryStream.h>
#include <Poco/StreamCopier.h>
#include <Poco/StringTokenizer.h>
#include <Storages/MutableSupport.h>
#include <TiDB/Collation/Collator.h>
#include <TiDB/Decode/DatumCodec.h>
#include <TiDB/Decode/JsonBinary.h>
#include <TiDB/Decode/Vector.h>
#include <TiDB/Schema/SchemaNameMapper.h>
Expand All @@ -36,6 +39,7 @@
#include <algorithm>
#include <cmath>
#include <magic_enum.hpp>
#include <string>

namespace DB
{
Expand Down Expand Up @@ -230,7 +234,13 @@ Field ColumnInfo::defaultValueToField() const
return DB::GenDefaultField(*this);
return Field();
}
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({ return getBitValue(bit_value.convert<String>()); });
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({
// When we got bit_value from tipb, we have decoded it.
auto is_int = bit_value.isInteger();
if (is_int)
return bit_value.convert<UInt64>();
return getBitValue(bit_value.convert<String>());
});
}
// Floating type.
case TypeFloat:
Expand All @@ -239,7 +249,14 @@ Field ColumnInfo::defaultValueToField() const
case TypeDate:
case TypeDatetime:
case TypeTimestamp:
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({ return DB::parseMyDateTime(value.convert<String>()); });
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({
auto is_int = value.isInteger();
if (is_int)
{
return value.convert<UInt64>();
}
return DB::parseMyDateTime(value.convert<String>());
zimulala marked this conversation as resolved.
Show resolved Hide resolved
});
case TypeVarchar:
case TypeTinyBlob:
case TypeMediumBlob:
Expand Down Expand Up @@ -277,12 +294,26 @@ Field ColumnInfo::defaultValueToField() const
return getDecimalValue(text);
});
case TypeTime:
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({ return getTimeValue(value.convert<String>()); });
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({
auto is_int = value.isInteger();
if (is_int)
{
return value.convert<UInt64>();
}
return getTimeValue(value.convert<String>());
});
case TypeYear:
// Never throw exception here, do not use TRY_CATCH_DEFAULT_VALUE_TO_FIELD
return getYearValue(value.convert<String>());
case TypeSet:
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({ return getSetValue(value.convert<String>()); });
TRY_CATCH_DEFAULT_VALUE_TO_FIELD({
auto is_int = value.isInteger();
if (is_int)
{
return value.convert<UInt64>();
}
return getSetValue(value.convert<String>());
});
case TypeTiDBVectorFloat32:
return genVectorFloat32Empty();
default:
Expand Down Expand Up @@ -1362,17 +1393,62 @@ ColumnInfo toTiDBColumnInfo(const tipb::ColumnInfo & tipb_column_info)
tidb_column_info.flag = tipb_column_info.flag();
tidb_column_info.flen = tipb_column_info.columnlen();
tidb_column_info.decimal = tipb_column_info.decimal();
tidb_column_info.collate = tipb_column_info.collation();
for (int i = 0; i < tipb_column_info.elems_size(); ++i)
tidb_column_info.elems.emplace_back(tipb_column_info.elems(i), i + 1);
// TiFlash get default value from origin_default_value, check `Field ColumnInfo::defaultValueToField() const`
// So we need to set origin_default_value to tipb_column_info.default_val()
// Related logic in tidb, https://github.com/pingcap/tidb/blob/45318da24d8e4c0c6aab836d291a33f949dd18bf/pkg/table/tables/tables.go#L2303-L2329
// For TypeBit, we need to set origin_default_bit_value to tipb_column_info.default_val().
if (tidb_column_info.tp == TypeBit)
tidb_column_info.origin_default_bit_value = tipb_column_info.default_val();
else
if (tidb_column_info.tp != TypeBit)
zimulala marked this conversation as resolved.
Show resolved Hide resolved
tidb_column_info.origin_default_value = tipb_column_info.default_val();
tidb_column_info.collate = tipb_column_info.collation();
for (int i = 0; i < tipb_column_info.elems_size(); ++i)
tidb_column_info.elems.emplace_back(tipb_column_info.elems(i), i + 1);
// Decode tipb_column_info.default_val.
{
// The default value is null.
if (tidb_column_info.origin_default_value.size() == 0)
Lloyd-Pottiger marked this conversation as resolved.
Show resolved Hide resolved
{
Poco::Dynamic::Var empty_val;
tidb_column_info.origin_default_value = empty_val;
return tidb_column_info;
}
size_t cursor = 0;
auto val = DB::DecodeDatum(cursor, tipb_column_info.default_val());
if (val.getType() == DB::Field::Types::String)
{
tidb_column_info.origin_default_value = val.get<String>();
return tidb_column_info;
}
switch (tidb_column_info.tp)
{
case TypeDate:
case TypeDatetime:
case TypeTimestamp:
case TypeSet:
tidb_column_info.origin_default_value = val.get<UInt64>();
break;
case TypeTime:
tidb_column_info.origin_default_value = val.get<Int64>();
break;
case TypeBit:
// For TypeBit, we need to set origin_default_bit_value to tipb_column_info.default_val().
tidb_column_info.default_bit_value = val.get<UInt64>();
JaySon-Huang marked this conversation as resolved.
Show resolved Hide resolved
break;
case TypeYear:
// If the year column has 'not null' option and the value is 0, this year value is '0000'.
if (val.get<Int64>() == 0)
{
Poco::Dynamic::Var empty_val;
tidb_column_info.origin_default_value = empty_val;
break;
}
Comment on lines +1422 to +1427
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// The above types will be processed again when defaultValueToField is called.
// By distinguishing the type of the string type(by synchronizing the default values in the schema, its type is string),
// we can distinguish whether it needs to be processed.
default:
auto str_val = DB::applyVisitor(DB::FieldVisitorToString(false), val);
tidb_column_info.origin_default_value = str_val;
}
}

return tidb_column_info;
}

Expand Down
25 changes: 25 additions & 0 deletions tests/fullstack-test2/ddl/alter_default_value.test
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,29 @@ mysql> set session tidb_isolation_read_engines='tiflash'; select /*+ read_from_s
| 1 | Unprioritized |
+------+---------------+

mysql> alter table test.t drop column e;

#############
# Alter table... add column with default value
mysql> alter table test.t add column col1 int default 30;
mysql> alter table test.t add column col2 year not null;
mysql> alter table test.t add column col3 decimal(6,2) not null;
mysql> alter table test.t add column col4 decimal(6,2) default "1.234";
mysql> alter table test.t add column col5 int default 123;
mysql> alter table test.t add column col6 varchar(255) default 'sss';
mysql> alter table test.t add column col7 timestamp default '2017-02-11';
mysql> alter table test.t add column col8 datetime default '2018-11-04 23:12:03';
mysql> alter table test.t add column col9 year default '2012';
mysql> alter table test.t add column col10 set('value1', 'value2', 'value3') default 'value3';
mysql> alter table test.t add column col11 enum('value1', 'value2', 'value3') default 'value1';
mysql> alter table test.t add column col12 time default '2017-02-23 12:18:30';
mysql> alter table test.t add column col13 time default null;
mysql> set session tidb_isolation_read_engines='tiflash'; select col1, col2, col3, col4, col5, col6, col7, col8, col9, col10, col11, col12, col13 from test.t;
+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+----------+-------+
| col1 | col2 | col3 | col4 | col5 | col6 | col7 | col8 | col9 | col10 | col11 | col12 | col13 |
+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+----------+-------+
| 30 | 0000 | 0.00 | 1.23 | 123 | sss | 2017-02-11 00:00:00 | 2018-11-04 23:12:03 | 2012 | value3 | value1 | 12:18:30 | NULL |
| 30 | 0000 | 0.00 | 1.23 | 123 | sss | 2017-02-11 00:00:00 | 2018-11-04 23:12:03 | 2012 | value3 | value1 | 12:18:30 | NULL |
+------+------+------+------+------+------+---------------------+---------------------+------+--------+--------+----------+-------+

mysql> drop table if exists test.t