Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Add the parser to parse time. #249

Closed
wants to merge 30 commits into from

Conversation

Shylock-Hg
Copy link
Contributor

No description provided.

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Sep 24, 2020
@Shylock-Hg Shylock-Hg requested review from a team September 24, 2020 07:20
return kDateDay;
}
if (t.type == TokenType::kNumber) {
if (t.val < 1 || t.val > 31) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Inaccurate judgment, need to judge with the month.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only limit checking, when create date will check the date validation.

darionyaphet
darionyaphet previously approved these changes Oct 19, 2020
@Shylock-Hg Shylock-Hg requested review from laura-ding and a team October 22, 2020 06:25
@CLAassistant
Copy link

CLAassistant commented Mar 26, 2021

CLA assistant check
All committers have signed the CLA.

@monadbobo
Copy link
Contributor

There are two problems:

  1. does our current timestamp not support integers anymore?
  2. if it's a string, which ISO standard are we parse according to?

@Shylock-Hg
Copy link
Contributor Author

There are two problems:

  1. does our current timestamp not support integers anymore?
  2. if it's a string, which ISO standard are we parse according to?
  1. The pr don't affect the timestamp
  2. Follow the opencypher

@Shylock-Hg Shylock-Hg requested a review from darionyaphet April 26, 2021 03:48

// TODO(shylock) support weeks/days format when support the convertion
// The state of current parser
enum State {
Copy link
Contributor

Choose a reason for hiding this comment

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

enum class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used for array index.

tokens_.emplace_back(t);
digits.clear();
}
} else if (kTimeDelimiter == *c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's more intuitive to use switch cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a condition isdigit.

// State, ShiftMap
{kInitial,
[](Token, Token, Context &ctx) -> StatusOr<State> {
if (ctx.type == ExpectType::kDateTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use a hashmap 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.

The array is more efficient.

nebula::time::TimeParser parser;
auto result = parser.parseDateTime("2019-01-03T22:22:3.2333");
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(nebula::DateTime(2019, 1, 3, 22, 22, 3, 2333), result.value());
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be 233300 microseconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it would be a utc time?

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 it's just the utc time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. The process of fraction of second had be corrected.

@Shylock-Hg Shylock-Hg requested review from monadbobo, CPWstatic and a team June 3, 2021 09:01
namespace nebula {
namespace time {

/*static*/ const std::vector<TimeParser::Component> TimeParser::dateTimeStates = {
Copy link
Contributor

Choose a reason for hiding this comment

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

please try to use FSM to solve these states transfer issue, that's powerful tool for string parser, need not any branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about your meaning. Could you detail the structure about the FSM you means, and how to replace the branches.

@Shylock-Hg Shylock-Hg requested a review from yixinglu July 19, 2021 08:25
}

std::string DateTime::toString() const {
auto microsecStr = folly::stringPrintf("%.9f", static_cast<uint32_t>(microsec) / 1000000.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 do we keep 9 digits for ms instead of 6? Nebula's time precision is different from openCypher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the difference? They are both fraction of second.

@Shylock-Hg Shylock-Hg requested review from Aiee and a team July 20, 2021 02:13
@Shylock-Hg
Copy link
Contributor Author

Move to vesoft-inc/nebula#2770

@Shylock-Hg Shylock-Hg closed this Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants