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

Feature/bison datetime parser #3179

Merged
merged 36 commits into from
Dec 13, 2021

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Oct 21, 2021

Refactor #2770 by bison.

Close #2720
Close #2630

@Shylock-Hg Shylock-Hg added ready-for-testing PR: ready for the CI test doc affected PR: improvements or additions to documentation labels Oct 21, 2021
@Shylock-Hg Shylock-Hg mentioned this pull request Oct 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #3179 (7a39eff) into master (67bb6ff) will increase coverage by 0.01%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3179      +/-   ##
==========================================
+ Coverage   85.32%   85.34%   +0.01%     
==========================================
  Files        1289     1293       +4     
  Lines      119819   120024     +205     
==========================================
+ Hits       102239   102430     +191     
- Misses      17580    17594      +14     
Impacted Files Coverage Δ
src/common/time/TimezoneInfo.cpp 50.00% <ø> (+4.54%) ⬆️
src/daemons/GraphDaemon.cpp 65.51% <50.00%> (-0.56%) ⬇️
src/daemons/MetaDaemon.cpp 62.62% <50.00%> (-0.27%) ⬇️
src/daemons/StorageDaemon.cpp 62.22% <50.00%> (-0.57%) ⬇️
src/common/datatypes/test/ValueToJsonTest.cpp 88.19% <64.28%> (-0.56%) ⬇️
src/common/time/TimeUtils.h 84.00% <80.00%> (-4.58%) ⬇️
src/common/time/parser/DatetimeReader.cpp 97.22% <97.22%> (ø)
src/common/time/parser/test/DateTimeParserTest.cpp 99.19% <99.19%> (ø)
src/common/datatypes/Date.cpp 100.00% <100.00%> (ø)
src/common/datatypes/Date.h 72.82% <100.00%> (+3.69%) ⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a0fbb6...7a39eff. Read the comment docs.

@Shylock-Hg Shylock-Hg requested a review from jievince October 22, 2021 02:13
@Shylock-Hg Shylock-Hg requested a review from jievince October 22, 2021 02:26
src/common/time/parser/DatetimeScanner.h Outdated Show resolved Hide resolved
src/common/time/parser/DatetimeScanner.h Show resolved Hide resolved
;

date
: INTEGER NEGATIVE INTEGER NEGATIVE INTEGER {
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 parser to validate the date value? It's more clear to replace the INTEGER with meaningful YEAR to validate the format. You can define the YEAR with regex {DEC}{1,4} to do the same thing.

Copy link
Contributor Author

@Shylock-Hg Shylock-Hg Oct 27, 2021

Choose a reason for hiding this comment

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

We still need to check the value range, e.g. 99 is still invalid minute number. So do it in one place now.

Copy link
Contributor

Choose a reason for hiding this comment

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

minutes could also use regex to validate. you have used the parser why not let it do more thing?

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 too complicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And different month has different count of days, it can't validate by lex/parse rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

minutes could also use regex to validate. you have used the parser why not let it do more thing?

Where is this check? I don't find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See validateDate function

jievince
jievince previously approved these changes Nov 24, 2021
Aiee
Aiee previously approved these changes Nov 25, 2021
@Shylock-Hg Shylock-Hg dismissed stale reviews from Aiee and jievince via 29c40d3 November 30, 2021 03:00
@Shylock-Hg Shylock-Hg requested review from Aiee and jievince November 30, 2021 03:01
@yixinglu yixinglu merged commit 393e169 into vesoft-inc:master Dec 13, 2021
@HarrisChu HarrisChu added the incompatible PR: incompatible with the recently released version label Jan 11, 2022
@HarrisChu
Copy link
Contributor

incompatible with before.
should info users that timestamp just means time in seconds

(root@nebula) [s1]> yield timestamp("2020-01-11T19:28:23")
+----------------------------------+
| timestamp("2020-01-11T19:28:23") |
+----------------------------------+
| 1578770903                       |
+----------------------------------+
Got 1 rows (time spent 3166/14662 us)

Tue, 11 Jan 2022 16:29:18 CST

(root@nebula) [s1]> yield timestamp("2020-01-11T19:28:23.123456")
+-----------------------------------------+
| timestamp("2020-01-11T19:28:23.123456") |
+-----------------------------------------+
| BAD_DATA                                |
+-----------------------------------------+
Got 1 rows (time spent 2434/14832 us)

Tue, 11 Jan 2022 16:29:28 CST
(root@nebula) [s1]> yield timestamp(1641890171222)
+--------------------------+
| timestamp(1641890171222) |
+--------------------------+
| BAD_DATA                 |
+--------------------------+
Got 1 rows (time spent 2637/13288 us)

Tue, 11 Jan 2022 16:36:51 CST

(root@nebula) [s1]> yield timestamp(1641890171)
+-----------------------+
| timestamp(1641890171) |
+-----------------------+
| 1641890171            |
+-----------------------+
Got 1 rows (time spent 2490/10609 us)

Tue, 11 Jan 2022 16:36:53 CST

@Shylock-Hg
Copy link
Contributor Author

incompatible with before. should info users that timestamp just means time in seconds

(root@nebula) [s1]> yield timestamp("2020-01-11T19:28:23")
+----------------------------------+
| timestamp("2020-01-11T19:28:23") |
+----------------------------------+
| 1578770903                       |
+----------------------------------+
Got 1 rows (time spent 3166/14662 us)

Tue, 11 Jan 2022 16:29:18 CST

(root@nebula) [s1]> yield timestamp("2020-01-11T19:28:23.123456")
+-----------------------------------------+
| timestamp("2020-01-11T19:28:23.123456") |
+-----------------------------------------+
| BAD_DATA                                |
+-----------------------------------------+
Got 1 rows (time spent 2434/14832 us)

Tue, 11 Jan 2022 16:29:28 CST
(root@nebula) [s1]> yield timestamp(1641890171222)
+--------------------------+
| timestamp(1641890171222) |
+--------------------------+
| BAD_DATA                 |
+--------------------------+
Got 1 rows (time spent 2637/13288 us)

Tue, 11 Jan 2022 16:36:51 CST

(root@nebula) [s1]> yield timestamp(1641890171)
+-----------------------+
| timestamp(1641890171) |
+-----------------------+
| 1641890171            |
+-----------------------+
Got 1 rows (time spent 2490/10609 us)

Tue, 11 Jan 2022 16:36:53 CST

This PR don't modify this.

@HarrisChu HarrisChu removed the incompatible PR: incompatible with the recently released version label Jan 11, 2022
@Shylock-Hg Shylock-Hg deleted the feature/bison-datetime-parser branch February 9, 2022 07:17
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
8 participants