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

Add millisecond field for time map initialize. #2781

Merged

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Sep 2, 2021

Close #2612

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Sep 2, 2021
@Shylock-Hg Shylock-Hg requested review from a team September 2, 2021 03:54
@yixinglu
Copy link
Contributor

yixinglu commented Sep 2, 2021

Please add some integration tests in TCK

@Shylock-Hg
Copy link
Contributor Author

Please add some integration tests in TCK

Done

@@ -55,11 +55,16 @@ constexpr int64_t kMaxTimestamp = std::numeric_limits<int64_t>::max() / 10000000
return Status::Error("Invalid second number `%ld'.", kv.second.getInt());
}
dt.sec = kv.second.getInt();
} else if (kv.first == "millisecond") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use switch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switch can't process string type.

tag_date.f_datetime = DateTime("2018-03-04T22:30:40")
tag_date.f_date = Date({year: 2018, month: 3, day: 4}),
tag_date.f_time = Time({hour: 22, minute: 1, second: 0, millisecond: 0, microsecond: 0}),
tag_date.f_datetime = DateTime({year: 2018, month: 3, day: 4, hour: 22, minute: 30, second: 40, millisecond: 0, microsecond: 0})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add a new test case instead of modifying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The origin case is tested in other case too.

@Shylock-Hg Shylock-Hg requested review from CPWstatic and a team September 7, 2021 08:57
yixinglu
yixinglu previously approved these changes Sep 10, 2021
@Shylock-Hg Shylock-Hg requested a review from a team September 10, 2021 02:39
Aiee
Aiee previously approved these changes Sep 15, 2021
@yixinglu yixinglu self-requested a review September 15, 2021 16:53
@Sophie-Xie Sophie-Xie removed the request for review from a team September 16, 2021 05:08
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021
@Shylock-Hg Shylock-Hg dismissed stale reviews from Aiee and yixinglu via 87241fe September 23, 2021 05:49
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

LGTM

@yixinglu yixinglu merged commit d968b44 into vesoft-inc:master Sep 24, 2021
@yixinglu yixinglu added the doc affected PR: improvements or additions to documentation label Sep 24, 2021
@Shylock-Hg Shylock-Hg deleted the correct/time-millisecond-initialize branch February 9, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting millisecond failed but microsecond worked
5 participants