fix datetime format #4524

caton-hpg · 2022-08-16T02:30:51Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number: #4487

Description:

It will be BAD_DATA if input a bad year or month.

(root@nebula) [(none)]> return datetime("32767");
+-----------------------------+
| datetime("32767")           |
+-----------------------------+
| 32767-01-01T00:00:00.000000 |
+-----------------------------+
Got 1 rows (time spent 1900/2890 us)


(root@nebula) [(none)]> return datetime("32768");
+-------------------+
| datetime("32768") |
+-------------------+
| BAD_DATA          |
+-------------------+
Got 1 rows (time spent 1265/1800 us)


(root@nebula) [(none)]> return datetime("-32768");
+------------------------------+
| datetime("-32768")           |
+------------------------------+
| -32768-01-01T00:00:00.000000 |
+------------------------------+
Got 1 rows (time spent 1127/1800 us)


(root@nebula) [(none)]> return datetime("-32769");
+--------------------+
| datetime("-32769") |
+--------------------+
| BAD_DATA           |
+--------------------+
Got 1 rows (time spent 1084/1699 us)

How do you solve it?

year is in [-32768,32767].

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

Shylock-Hg · 2022-08-16T02:34:12Z

I think it's an out of range date, so you could check range and return error. Don't modify parser (and test).

codecov-commenter · 2022-08-17T08:22:47Z

Codecov Report

Merging #4524 (0d7e1ab) into master (c884477) will increase coverage by 0.02%.
The diff coverage is 95.50%.

@@            Coverage Diff             @@
##           master    #4524      +/-   ##
==========================================
+ Coverage   84.66%   84.68%   +0.02%     
==========================================
  Files        1357     1357              
  Lines      135081   135150      +69     
==========================================
+ Hits       114360   114456      +96     
+ Misses      20721    20694      -27

Impacted Files	Coverage Δ
src/parser/test/ParserTest.cpp	`100.00% <ø> (ø)`
src/clients/meta/MetaClient.cpp	`76.18% <84.61%> (-0.20%)`	⬇️
src/storage/test/GetPropTest.cpp	`95.34% <94.59%> (-0.05%)`	⬇️
src/clients/meta/MetaClient.h	`92.30% <100.00%> (ø)`
src/common/function/test/FunctionManagerTest.cpp	`98.84% <100.00%> (ø)`
src/common/time/TimeUtils.cpp	`89.40% <100.00%> (ø)`
src/common/time/parser/test/DateTimeParserTest.cpp	`99.52% <100.00%> (+0.06%)`	⬆️
src/storage/exec/QueryUtils.h	`94.01% <100.00%> (+0.10%)`	⬆️
src/graph/executor/StorageAccessExecutor.h	`57.77% <0.00%> (-8.89%)`	⬇️
src/graph/context/Result.cpp	`70.00% <0.00%> (-7.78%)`	⬇️
... and 38 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

caton-hpg · 2022-08-18T03:26:38Z

Fixed.

year is in [0, 9999]
month is in [1, 12]

Shylock-Hg · 2022-08-18T03:26:45Z

src/common/time/TimeUtils.h

@@ -44,6 +44,14 @@ class TimeUtils {
    if ((p[date.month] - p[date.month - 1]) < date.day) {
      return Status::Error("`%s' is not a valid date.", date.toString().c_str());
    }
+    if (date.year < 0 || date.year > 9999) {


Check it in parser, greater than 9999 is also valid, our upper bound is int32_t::max().

Lower bound too.

I talk with @HarrisChu .
He thinks date.year should be year. Should not be int32_t::max().

It's obviously incorrect, we should provide time range as wide as possible for user. You could check Postgres in https://www.postgresql.org/docs/9.1/datatype-datetime.html or other databases.

what's your expected for below queries:

yield datetime(-1) yield datetime(1000) yield datetime(123456) yield datetime("-1") yield datetime("1000") yield datetime("123456")

All valid date.

Negative date means B.C.

one is for epoch, the other is just year

datetime("-1") is unexpected.

fixed. year is in [-32767,32767].

Shylock-Hg · 2022-08-24T09:45:45Z

src/common/time/TimeUtils.cpp

@@ -25,7 +25,8 @@ constexpr int64_t kMaxTimestamp = std::numeric_limits<int64_t>::max() / 10000000
      return Status::Error("Invalid value type.");
    }
    if (kv.first == "year") {
-      if (kv.second.getInt() < std::numeric_limits<int16_t>::min() ||
+      // year should be in [-32767, 32767] and same as parser
+      if (kv.second.getInt() <= std::numeric_limits<int16_t>::min() ||


Why less and equal?

The max of year in paser is 32767. The min is -32767.
In order to be same as parser.

Where limit this in parser?

The year, month, day, hour and others are INTEGER.
INTEGER is a [0-9]+ and cannot be a negative int.
Negative year supports by

| NEGATIVE INTEGER NEGATIVE INTEGER NEGATIVE INTEGER { $$ = new nebula::Date(0-$2, $4, $6);

I can't catch you.

fixed. year is in [-32768,32767].

Shylock-Hg · 2022-08-25T09:35:07Z

We'd better avoid force-push

codesigner

Nice

jinyingsunny · 2022-10-18T12:35:40Z

ent-3.3 checked

caton-hpg requested review from dutor and codesigner as code owners August 16, 2022 02:30

Sophie-Xie requested a review from Shylock-Hg August 16, 2022 02:31

caton-hpg force-pushed the datetime_format branch from ac0b822 to 43c988a Compare August 17, 2022 05:44

Sophie-Xie added the ready-for-testing PR: ready for the CI test label Aug 17, 2022

caton-hpg force-pushed the datetime_format branch from 43c988a to d2b90e4 Compare August 18, 2022 03:21

Shylock-Hg reviewed Aug 18, 2022

View reviewed changes

caton-hpg force-pushed the datetime_format branch 2 times, most recently from da33162 to b114ad1 Compare August 24, 2022 09:36

Shylock-Hg reviewed Aug 24, 2022

View reviewed changes

caton-hpg force-pushed the datetime_format branch 2 times, most recently from 9c4d504 to 706b471 Compare August 25, 2022 05:31

Shylock-Hg previously approved these changes Aug 25, 2022

View reviewed changes

caton-hpg added 8 commits August 25, 2022 17:33

check date

5969c59

fix year range

b1523df

fix year range of date

f0a666a

fix DateTimeParserTest

2c617b2

fix function_manager_test

229d8c0

validate month

78619a6

validate year

c050317

first check month

f245c02

caton-hpg dismissed Shylock-Hg’s stale review via f245c02 August 25, 2022 09:34

caton-hpg force-pushed the datetime_format branch from 706b471 to f245c02 Compare August 25, 2022 09:34

Sophie-Xie and others added 2 commits August 26, 2022 10:18

Merge branch 'master' into datetime_format

4622c50

Merge branch 'master' into datetime_format

62298eb

Shylock-Hg approved these changes Aug 30, 2022

View reviewed changes

codesigner approved these changes Aug 30, 2022

View reviewed changes

Sophie-Xie merged commit 474554f into vesoft-inc:master Aug 30, 2022

wey-gu mentioned this pull request Sep 3, 2022

Weekly Report 2022-09-02 vesoft-inc/nebula-community#130

Closed

foesa-yang added the doc affected PR: improvements or additions to documentation label Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix datetime format #4524

fix datetime format #4524

caton-hpg commented Aug 16, 2022 •

edited

Loading

Shylock-Hg commented Aug 16, 2022 •

edited

Loading

codecov-commenter commented Aug 17, 2022 •

edited

Loading

caton-hpg commented Aug 18, 2022

Shylock-Hg Aug 18, 2022

Shylock-Hg Aug 18, 2022

caton-hpg Aug 18, 2022

Shylock-Hg Aug 18, 2022

HarrisChu Aug 18, 2022

Shylock-Hg Aug 18, 2022

Shylock-Hg Aug 18, 2022

HarrisChu Aug 19, 2022

Shylock-Hg Aug 19, 2022

caton-hpg Aug 23, 2022

Shylock-Hg Aug 24, 2022

caton-hpg Aug 25, 2022

Shylock-Hg Aug 25, 2022

caton-hpg Aug 25, 2022

Shylock-Hg Aug 25, 2022

caton-hpg Aug 25, 2022

Shylock-Hg commented Aug 25, 2022

codesigner left a comment

jinyingsunny commented Oct 18, 2022

fix datetime format #4524

fix datetime format #4524

Conversation

caton-hpg commented Aug 16, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number: #4487

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

Shylock-Hg commented Aug 16, 2022 • edited Loading

codecov-commenter commented Aug 17, 2022 • edited Loading

Codecov Report

caton-hpg commented Aug 18, 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

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

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

Choose a reason for hiding this comment

Shylock-Hg commented Aug 25, 2022

codesigner left a comment

Choose a reason for hiding this comment

jinyingsunny commented Oct 18, 2022

caton-hpg commented Aug 16, 2022 •

edited

Loading

Shylock-Hg commented Aug 16, 2022 •

edited

Loading

codecov-commenter commented Aug 17, 2022 •

edited

Loading