-
Notifications
You must be signed in to change notification settings - Fork 409
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
[flash 411] use compatible new datetime/ date #224
Conversation
/run-integration-tests |
/run-integration-tests |
1 similar comment
/run-integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can try string_view
and std::move
.
return idx; | ||
} | ||
|
||
std::vector<String> parseDateFormat(String format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use const ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trimInPlace is defined as template<S> S& trimInPlace(S& str)
. If I pass a const ref, it will cause a compile error
|
||
using FunctionToYYYYMM = FunctionDateOrDateTimeToSomething<DataTypeUInt32, ToYYYYMMImpl>; | ||
using FunctionToYYYYMMDD = FunctionDateOrDateTimeToSomething<DataTypeUInt32, ToYYYYMMDDImpl>; | ||
using FunctionToYYYYMMDDhhmmss = FunctionDateOrDateTimeToSomething<DataTypeUInt64, ToYYYYMMDDhhmmssImpl>; | ||
|
||
using FunctionAddSeconds = FunctionDateOrDateTimeAddInterval<AddSecondsImpl>; | ||
using FunctionAddMinutes = FunctionDateOrDateTimeAddInterval<AddMinutesImpl>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like all the existing date/datetime udfs do not work for new datetime/date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
{ | ||
|
||
const char * s = buf.position(); | ||
if (s + 19 <= buf.buffer().end()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the pessimistic path?
UInt64 ymd = ((UInt64)values.year * 13 + values.month) << 5 | values.day_of_month; | ||
UInt64 hms = (UInt64)hour << 12 | minute << 6 | second; | ||
copy = (ymd << 17 | hms) << 24; | ||
auto plain_text = orig.safeGet<String>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why get String from the orig filed? The orig field is the internal representation in TiFlash, and TiFlash does not use String to represent the date/datetime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used by test. User insert datetime/date by string literal
@@ -43,14 +43,14 @@ using DB::Timestamp; | |||
M(Float, 4, Float, Float32, false) \ | |||
M(Double, 5, Float, Float64, false) \ | |||
M(Null, 6, Nil, Nothing, false) \ | |||
M(Timestamp, 7, Int, DateTime, false) \ | |||
M(Timestamp, 7, Int, MyDateTime, false) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why timestamp is mapped to MyDateTime instead of MyTimestamp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has not implemented
Does this pr support session level timezone? And in Mysql/TiDB only the values of the Timestamp data type is affected by time zone, does TiFlash follow this principle? |
String toString() const; | ||
}; | ||
|
||
struct MyDate : public MyTimeBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MyDate
inherits from MyTimeBase
may introduce too much overhead. Can we use a lightweight class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TiDB use uint64 to store date type, we can optimize it in the future.
@windtalker I will support timestamp and timezone in next pr |
/rebuild |
/run-integration-tests tiflash=hanfei-new-datetime |
1 similar comment
/run-integration-tests tiflash=hanfei-new-datetime |
@windtalker PTAL again |
MyDate, | ||
MyDateTime, | ||
MyTimeStamp, | ||
MyTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MyTime is mapping TypeDuration in MyTimeType enum? Better to use the same suffix, MyTime => MyDuration or TypeDuration => TypeTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep consistent with MySQL Grammar
DECLARE_DICT_GET_TRAITS(UInt32, DataTypeDate) | ||
DECLARE_DICT_GET_TRAITS(Int64, DataTypeDateTime) | ||
DECLARE_DICT_GET_TRAITS(UInt16, DataTypeDate) | ||
DECLARE_DICT_GET_TRAITS(UInt32, DataTypeDateTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why new type is not declared here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionsExternalDictionaries.h
has no relation with our own logic.
/run-integration-tests tiflash=hanfei-new-datetime |
@@ -17,96 +18,19 @@ using DB::Field; | |||
template <TP tp, typename = void> | |||
struct DatumOp | |||
{ | |||
static void unflatten(const Field &, std::optional<Field> &) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove unflattern
/flattern
methods, we are using them to deal with sign mismatch of Enum
type between TiDB and TiFlash.
Just remove the unnecessary specialization version of Datum
for Date
/DateTime
.
/run-integration-tests tiflash=hanfei-new-datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-integration-tests tiflash=hanfei-new-datetime |
Signed-off-by: Wish <breezewish@outlook.com> Co-authored-by: JaySon <tshent@qq.com>
Signed-off-by: Wish <breezewish@outlook.com> Co-authored-by: JaySon <tshent@qq.com>
Signed-off-by: Wish <breezewish@outlook.com> Co-authored-by: JaySon <tshent@qq.com>
No description provided.