Support Extract DateTimeFromString #6300

birdstorm · 2022-11-11T10:40:31Z

What problem does this PR solve?

Issue number: close #6304

Support ExtractDateTimeFromString Sig.

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
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

None

Signed-off-by: birdstorm <[email protected]>

ti-chi-bot · 2022-11-11T10:40:32Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

mengxin9014
ywqzzy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: birdstorm <[email protected]>

birdstorm · 2022-11-16T06:16:18Z

/run-all-tests

xzhangxian1008 · 2022-11-17T02:32:46Z

/assign

dbms/src/Functions/FunctionsDateTime.h

dbms/src/Functions/FunctionsDuration.h

dbms/src/Functions/FunctionsDateTime.h

dbms/src/Functions/tests/gtest_datetime_extract.cpp

dbms/src/Common/StringUtils/StringUtils.h

Signed-off-by: birdstorm <[email protected]>

birdstorm · 2022-11-17T10:25:41Z

dbms/src/Common/tests/gtest_mytime.cpp

+        {"121231113045", "2012-12-31 11:30:45"},
+        {"2012-02-29", "2012-02-29 00:00:00"},
+        {"00-00-00", "0000-00-00 00:00:00"},
+        // {"00-00-00 00:00:00.123", "2000-00-00 00:00:00.123"},


Test cases use fixed fsp; automatically calculating the fsp of a String is not implemented yet. I think we can leave these cases for later.

birdstorm · 2022-11-17T10:27:12Z

dbms/src/Functions/tests/gtest_datetime_extract.cpp

+        {" \t\r2012^12^31T11+30+45 \n ", {31113045000000, 31113045, 311130, 3111}},
+        {"20121231113045", {31113045000000, 31113045, 311130, 3111}},
+        {"121231113045", {31113045000000, 31113045, 311130, 3111}},
+        // {"1701020304.1", {2030401000000, 2030401, 20304, 203}},


Parsing for float string is not implemented yet. It is not going to be done in this PR.

birdstorm · 2022-11-17T10:54:00Z

/run-all-tests

xzhangxian1008 · 2022-11-18T08:05:02Z

dbms/src/Functions/FunctionsDateTime.h

                fmt::format("Illegal type {} of second argument of function {}. Must be DateOrDateTime.", arguments[1]->getName(), getName()),
-                Errors::Coprocessor::BadRequest);
+                ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT);

        return std::make_shared<DataTypeInt64>();


Maybe we should return nullable type as some extraction may fail.

consider doing it in another PR.

dbms/src/Common/StringUtils/StringUtils.h

dbms/src/Functions/FunctionsDateTime.h

dbms/src/Common/MyTime.cpp

xzhangxian1008 · 2022-11-23T03:17:19Z

dbms/src/Functions/FunctionsDateTime.h

+        return signMultiplier(duration) * (duration.hours() * 10000LL + duration.minutes() * 100LL + duration.seconds());
+    }
+
+    static Int64 extractDayMinute(Int64 nano)


How about overloading this function with static Int64 extractDayMinute(const MyDuration &) so that we can directly pass MyDuration value to the function and reduce usless construction and destruction.

Not only this but all functions similar to this function

I think this is not necessary because functions named extractDayMinute have different approaches. The DateTime and the Duration are not stored in the same format, they have different encoding formats and implementations. Moving these logics together may confuse readers.

xzhangxian1008 · 2022-11-23T03:27:14Z

dbms/src/Functions/FunctionsDateTime.h

+        result = ExtractMyDurationImpl::extractDayMinute(duration.nanoSecond());
+
+        Field datetime_field = parseMyDateTime(dtStr);
+        if (!datetime_field.isNull())


It seems that we prefer to return value which is parsed by parseMyDateTime, how about moving codes related with paring duration to the tail of codes that parse datetime?

Not only in this function but also all similar functions

We prefer to return value that is equivalently parsed by parseMyDuration and parseMyDateTime

reference:

https://github.com/pingcap/tidb/blob/6feaa527160499ca646f7def098e4ea788b53113/types/time.go#L1802-L1824

We prefer to return value that is equivalently parsed by parseMyDuration and parseMyDateTime

okk

Signed-off-by: birdstorm <[email protected]>

birdstorm · 2022-11-28T02:37:12Z

/run-all-tests

xzhangxian1008 · 2022-12-01T01:55:37Z

lgtm

mengxin9014

Others LGTM

dbms/src/Common/StringUtils/StringUtils.h

ywqzzy

Rest LGTM

dbms/src/Common/StringUtils/StringUtils.h

birdstorm · 2022-12-01T04:09:48Z

/merge

ti-chi-bot · 2022-12-01T04:09:49Z

@birdstorm: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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 · 2022-12-01T04:09:51Z

This pull request has been accepted and is ready to merge.

Commit hash: 367914f

support extract datetime from string

f28c928

Signed-off-by: birdstorm <[email protected]>

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 11, 2022

format

a6df07d

Signed-off-by: birdstorm <[email protected]>

ti-chi-bot removed the do-not-merge/needs-linked-issue label Nov 14, 2022

Merge branch 'master' into support-extract-datetime-string

a749628

birdstorm mentioned this pull request Nov 14, 2022

expression: Support Push Down All Extract Function to TiFlash pingcap/tidb#39104

Open

3 tasks

birdstorm added 3 commits November 16, 2022 12:10

fix tests

f625e3d

Signed-off-by: birdstorm <[email protected]>

Merge branch 'master' into support-extract-datetime-string

0a5e346

add overflow test

f9cadfa

Signed-off-by: birdstorm <[email protected]>

birdstorm force-pushed the support-extract-datetime-string branch from ac7785b to f9cadfa Compare November 16, 2022 05:26

minor

81c21db

Signed-off-by: birdstorm <[email protected]>

ti-chi-bot assigned xzhangxian1008 Nov 17, 2022

windtalker requested review from mengxin9014, LittleFall and xzhangxian1008 November 17, 2022 02:33

windtalker assigned birdstorm and unassigned xzhangxian1008 Nov 17, 2022

ywqzzy reviewed Nov 17, 2022

View reviewed changes

Merge branch 'master' into support-extract-datetime-string

e3951bf

mengxin9014 reviewed Nov 17, 2022

View reviewed changes

dbms/src/Common/StringUtils/StringUtils.h Show resolved Hide resolved

birdstorm added 2 commits November 17, 2022 18:20

resolve comments

a14fa45

Signed-off-by: birdstorm <[email protected]>

Merge branch 'master' into support-extract-datetime-string

da3fe42

birdstorm commented Nov 17, 2022

View reviewed changes

xzhangxian1008 reviewed Nov 18, 2022

View reviewed changes

xzhangxian1008 reviewed Nov 23, 2022

View reviewed changes

birdstorm added 3 commits November 26, 2022 16:43

Merge branch 'master' into support-extract-datetime-string

b621855

resolve comments

88eaceb

Signed-off-by: birdstorm <[email protected]>

Merge branch 'master' into support-extract-datetime-string

9c1ebfc

mengxin9014 approved these changes Dec 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 1, 2022

ywqzzy approved these changes Dec 1, 2022

View reviewed changes

dbms/src/Common/StringUtils/StringUtils.h Outdated Show resolved Hide resolved

dbms/src/Common/StringUtils/StringUtils.h Outdated Show resolved Hide resolved

dbms/src/Common/StringUtils/StringUtils.h Outdated Show resolved Hide resolved

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 1, 2022

birdstorm added 2 commits December 1, 2022 13:08

resolve comments

7cbd833

Merge branch 'master' into support-extract-datetime-string

367914f

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2022

ti-chi-bot merged commit a460f48 into pingcap:master Dec 1, 2022

birdstorm deleted the support-extract-datetime-string branch December 1, 2022 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Extract DateTimeFromString #6300

Support Extract DateTimeFromString #6300

birdstorm commented Nov 11, 2022 •

edited

Loading

ti-chi-bot commented Nov 11, 2022 •

edited

Loading

birdstorm commented Nov 16, 2022

xzhangxian1008 commented Nov 17, 2022

birdstorm Nov 17, 2022

birdstorm Nov 17, 2022

birdstorm commented Nov 17, 2022

xzhangxian1008 Nov 18, 2022

birdstorm Nov 26, 2022

xzhangxian1008 Nov 23, 2022

birdstorm Nov 26, 2022

xzhangxian1008 Nov 23, 2022

birdstorm Nov 26, 2022 •

edited

Loading

birdstorm Nov 26, 2022

xzhangxian1008 Nov 28, 2022

birdstorm commented Nov 28, 2022

xzhangxian1008 commented Dec 1, 2022

mengxin9014 left a comment

ywqzzy left a comment

birdstorm commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

Support Extract DateTimeFromString #6300

Support Extract DateTimeFromString #6300

Conversation

birdstorm commented Nov 11, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 11, 2022 • edited Loading

birdstorm commented Nov 16, 2022

xzhangxian1008 commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birdstorm commented Nov 17, 2022

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

birdstorm Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birdstorm commented Nov 28, 2022

xzhangxian1008 commented Dec 1, 2022

mengxin9014 left a comment

Choose a reason for hiding this comment

ywqzzy left a comment

Choose a reason for hiding this comment

birdstorm commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

ti-chi-bot commented Dec 1, 2022

birdstorm commented Nov 11, 2022 •

edited

Loading

ti-chi-bot commented Nov 11, 2022 •

edited

Loading

birdstorm Nov 26, 2022 •

edited

Loading