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

[query] ParseTime function supports 95%+ of from / until time formats #2621

Merged
merged 27 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3fee32d
Added the ParseTimeReference function
teddywahle Sep 9, 2020
27f11a1
Completed the Parse ATTime functionality
teddywahle Sep 10, 2020
d98c5f1
Fixed broken regex
teddywahle Sep 10, 2020
3aaaee5
Merge branch 'master' into parse-time
teddywahle Sep 10, 2020
894d8f2
Update src/query/graphite/graphite/timespec.go
teddywahle Sep 10, 2020
097164c
Merge branch 'master' into parse-time
teddywahle Sep 10, 2020
15f5f68
Merge branch 'master' into parse-time
teddywahle Sep 21, 2020
a8795c3
Update src/query/graphite/graphite/timespec.go
teddywahle Sep 21, 2020
c405440
Apply suggestions from code review
teddywahle Sep 21, 2020
abaa0c0
Apply suggestions from code review
teddywahle Sep 21, 2020
858d337
Apply suggestions from code review
teddywahle Sep 21, 2020
55b55c1
ran go fmt
teddywahle Sep 21, 2020
6f007ed
Added bounds check
teddywahle Sep 21, 2020
b91f6cd
Merge branch 'master' into parse-time
teddywahle Sep 21, 2020
31c1948
Merge branch 'master' into parse-time
teddywahle Sep 23, 2020
a871c89
Update src/query/graphite/graphite/timespec_test.go
teddywahle Sep 23, 2020
93fdbc4
Merge branch 'master' into parse-time
teddywahle Oct 1, 2020
c520419
Apply suggestions from code review
teddywahle Oct 1, 2020
5a49e80
Apply suggestions from code review
teddywahle Oct 1, 2020
6d16b08
Added some (not all) of necessary changes to parse time
teddywahle Oct 1, 2020
eb6eaf7
Added more crons and error checks
teddywahle Oct 1, 2020
caf1795
ran go fmt
teddywahle Oct 1, 2020
c2ae930
Merge branch 'master' into parse-time
teddywahle Oct 1, 2020
800b95e
fixed parse time
teddywahle Oct 2, 2020
d2b3adf
Merge branch 'master' into parse-time
teddywahle Oct 2, 2020
062814c
Merge branch 'master' into parse-time
teddywahle Oct 5, 2020
91cbaeb
added nit changes from code review
teddywahle Oct 5, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 190 additions & 1 deletion src/query/graphite/graphite/timespec.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ import (
"github.com/m3db/m3/src/query/graphite/errors"
)

var reRelativeTime = regexp.MustCompile(`(?i)^\-([0-9]+)(s|min|h|d|w|mon|y)$`)
var reRelativeTime = regexp.MustCompile(`(?i)^\-([0-9]+)(s|min|h|d|w|mon|y)$`) // allows -3min, -4d, etc.
var reTimeOffset = regexp.MustCompile(`(?i)^(\-|\+)([0-9]+)(s|min|h|d|w|mon|y)$`) // -3min, +3min, -4d, +4d
var reMonthAndDay = regexp.MustCompile(`(?i)^(january|february|march|april|may|june|july|august|september|october|november|december)([0-9]{1,2}?)$`)
var reDayOfWeek = regexp.MustCompile(`(?i)^(sunday|monday|tuesday|wednesday|thursday|friday|saturday)$`)

var periods = map[string]time.Duration{
"s": time.Second,
Expand All @@ -42,6 +45,31 @@ var periods = map[string]time.Duration{
"y": time.Hour * 24 * 365,
}

var weekdays = map[string]int {
"sunday" : 0,
"monday" : 1,
"tuesday" : 2,
"wednesday" : 3,
"thursday" : 4,
"friday" : 5,
"saturday" : 6,
}

var months = map[string]int {
"january" : 1,
"february" : 2,
"march" : 3,
"april" : 4,
"may" : 5,
"june" : 6,
"july" : 7,
"august" : 8,
"september" : 9,
"october" : 10,
"november" : 11,
"december" : 12,
}

// on Jan 2 15:04:05 -0700 MST 2006
var formats = []string{
"15:04_060102",
Expand All @@ -50,6 +78,7 @@ var formats = []string{
"15:04_02.01.06",
"02.01.06",
"01/02/06",
"01/02/2006",
"060102",
"20060102",
}
Expand Down Expand Up @@ -94,6 +123,7 @@ func ParseTime(s string, now time.Time, absoluteOffset time.Duration) (time.Time
return now.Add(-1 * time.Duration(timePast) * period), nil
}


teddywahle marked this conversation as resolved.
Show resolved Hide resolved
newS := bypassTimeParseBug(s)
for _, format := range formats {
t, err := time.Parse(format, newS)
Expand All @@ -108,9 +138,146 @@ func ParseTime(s string, now time.Time, absoluteOffset time.Duration) (time.Time
return time.Unix(n, 0).UTC(), nil
}

ref, offset := s, ""
if strings.Contains(s, "+") {
stringSlice := strings.Split(s, "+")
ref, offset = stringSlice[0], stringSlice[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs validation that len(stringSlice) == 2

offset = "+" + offset
teddywahle marked this conversation as resolved.
Show resolved Hide resolved
} else if strings.Contains(s, "-") {
stringSlice := strings.Split(s, "-")
ref, offset = stringSlice[0], stringSlice[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs validation that len(stringSlice) == 2

offset = "-" + offset
teddywahle marked this conversation as resolved.
Show resolved Hide resolved
}
err = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to make err = nil, it will be shadowed/rewritten below, it's not required to reset it 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.

OK, good to know. Thanks.

parsedReference, err := ParseTimeReference(ref, now)
robskillington marked this conversation as resolved.
Show resolved Hide resolved
parsedOffset, err := ParseOffset(offset)
if err == nil {
return parsedReference.Add(parsedOffset), err
}

teddywahle marked this conversation as resolved.
Show resolved Hide resolved
return now, err
}


func ParseTimeReference(ref string, now time.Time) (time.Time, error) {
Copy link
Collaborator

@robskillington robskillington Sep 25, 2020

Choose a reason for hiding this comment

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

Need to add comment for exported symbols. i.e. // ParseTimeReference ...

hour := now.Hour()
minute := now.Minute()
refDate := time.Time{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put these into a var block (so you don't need to explicitly declare refDate as zero value):

var (
  hour = now.Hour()
  minute = now.Minute()
  refDate time.Time
)

teddywahle marked this conversation as resolved.
Show resolved Hide resolved

if ref == "" || ref == "now" {
return now, nil
}

// check if the time ref fits an absolute time format
for _, format := range formats {
t, err := time.Parse(format, ref)
if err == nil {
return time.Date(t.Year(), t.Month(), t.Day(), hour, minute, 0, 0, now.Location()), nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this just return t, nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's much cleaner.

teddywahle marked this conversation as resolved.
Show resolved Hide resolved
}
}

rawRef := ref

// Time of Day Reference
i := strings.Index(ref,":")
if 0 < i && i < 3 {
newHour, err := strconv.ParseInt(ref[:i], 10, 0)
if err != nil {
return time.Time{}, err
}
hour = int(newHour)
newMinute, err := strconv.ParseInt(ref[i+1:i+3], 10, 32)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be bounds check i+FOO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I added a bounds check and a new test to make sure the bounds check is working.

if err != nil {
return time.Time{}, err
}
minute = int(newMinute)
ref = ref[i+3:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be bounds check i+FOO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I added a bounds check and a new test to make sure the bounds check is working.


if len(ref) >= 2 {
if ref[:2] == "am" {
ref = ref[2:]
} else if ref[:2] == "pm" {
hour = (hour + 12) % 24
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this valid for e.g. 23:00PM or should that be a parse error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I will throw an error and add a test case for this.

ref = ref[2:]
}
}
}

// Xam or XXam
i = strings.Index(ref,"am")
if 0 < i && i < 3 {
newHour, err := strconv.ParseInt(ref[:i], 10, 32)
if err != nil {
return time.Time{}, err
}
hour = int(newHour)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this parse something like 10:00pm6am?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Added a check and an error test case for this.

minute = 0
ref = ref[i+2:]
}


// Xpm or XXpm
i = strings.Index(ref, "pm")
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should probably be regexing aginst the initial ref right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I changed it to a regex.

if 0 < i && i < 3 {
newHour, err := strconv.ParseInt(ref[:i], 10, 32)
if err != nil {
return time.Time{}, err
}
hour = int((newHour + 12) % 24)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it correct to parse 99pm or better to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Added a check and an error test case for this.

minute = 0
ref = ref[i+2:]
}

if strings.HasPrefix(ref, "noon") {
hour,minute = 12,0
ref = ref[4:]
} else if strings.HasPrefix(ref, "midnight") {
hour,minute = 0,0
ref = ref[8:]
} else if strings.HasPrefix(ref, "teatime") {
hour,minute = 16,0
ref = ref[7:]
}

refDate = time.Date(now.Year(), now.Month(), now.Day(), hour, minute, 0, 0, now.Location())

// Day reference
if ref == "yesterday" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these match against the initial ref before it's been shortened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because this function needs to be able to parse "noon yesterday" which will actually be "noonyesterday" when passed in because whitespace gets stripped. Parsing the unshortened ref could break this.

refDate = refDate.Add(time.Hour * -24)
} else if ref == "tomorrow" {
refDate = refDate.Add(time.Hour * 24)
} else if ref == "today" {
return refDate, nil
} else if reMonthAndDay.MatchString(ref) { // monthDay (january10, may6, may06 etc.)
day := 0
monthString := ""
if val, err := strconv.ParseInt(ref[len(ref)-2:],10,64); err == nil {
day = int(val)
monthString = ref[:len(ref)-2]
} else if val, err := strconv.ParseInt(ref[len(ref)-1:],10,64); err == nil {
day = int(val)
monthString = ref[:len(ref)-1]
} else {
return refDate, errors.NewInvalidParamsError(fmt.Errorf("day of month required after month name"))
}
refDate = time.Date(refDate.Year(), time.Month(months[monthString]), day, hour, minute, 0, 0, refDate.Location())
} else if reDayOfWeek.MatchString(ref) { // DayOfWeek (Monday, etc)
expectedDay := weekdays[ref]
today := int(refDate.Weekday())
dayOffset := today - expectedDay
if dayOffset < 0 {
dayOffset += 7
}
offSetDuration := time.Duration(dayOffset)
refDate = refDate.Add(time.Hour * 24 * -1 * offSetDuration)
} else if ref != "" {
return time.Time{}, errors.NewInvalidParamsError(fmt.Errorf("unkown day reference %s", rawRef))
Copy link
Collaborator

Choose a reason for hiding this comment

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

"unkown" -> "unknown"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

teddywahle marked this conversation as resolved.
Show resolved Hide resolved
}

return refDate, nil
}


// ParseDuration parses a duration
func ParseDuration(s string) (time.Duration, error) {
if m := reRelativeTime.FindStringSubmatch(s); len(m) != 0 {
Expand All @@ -125,3 +292,25 @@ func ParseDuration(s string) (time.Duration, error) {

return 0, errors.NewInvalidParamsError(fmt.Errorf("invalid relative time %s", s))
}

// ParseOffset parses a time offset (like a duration, but can be 0 or positive)
func ParseOffset(s string) (time.Duration, error) {
if s == "" {
return time.Duration(0), nil
}

if m := reTimeOffset.FindStringSubmatch(s); len(m) != 0 {
parity := 1
if m[1] == "-" {
parity = -1
}
timeInteger, err := strconv.ParseInt(m[2], 10, 32)
if err != nil {
return 0, errors.NewInvalidParamsError(fmt.Errorf("invalid time offset %v", err))
}
period := periods[strings.ToLower(m[3])]
return period * time.Duration(timeInteger) * time.Duration(parity), nil
}

return 0, errors.NewInvalidParamsError(fmt.Errorf("invalid time offset %s", s))
}
91 changes: 91 additions & 0 deletions src/query/graphite/graphite/timespec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ func TestParseTime(t *testing.T) {
{"20140307", time.Date(2014, time.March, 7, 0, 0, 0, 0, time.UTC)},
{"140307", time.Date(2014, time.March, 7, 0, 0, 0, 0, time.UTC)},
{"1432581620", time.Date(2015, time.May, 25, 19, 20, 20, 0, time.UTC)},
{"now", time.Date(2013, time.April, 3, 4, 5, 0, 0, time.UTC)},
{"midnight", time.Date(2013, time.April, 3, 0, 0, 0, 0, time.UTC)},
{"midnight+1h", time.Date(2013, time.April, 3, 1, 0, 0, 0, time.UTC)},
{"april08+1d", time.Date(2013, time.April, 9, 4, 5, 0, 0, time.UTC)},
}

for _, test := range tests {
Expand Down Expand Up @@ -74,6 +78,27 @@ func TestParseDuration(t *testing.T) {
}
}

func TestParseOffset(t *testing.T) {
tests := []struct {
timespec string
expectedDuration time.Duration
}{
{"-4h", -4 * time.Hour},
{"-35MIN", -35 * time.Minute},
{"-10s", -10 * time.Second},
{"+4h", 4 * time.Hour},
{"+35MIN", 35 * time.Minute},
{"+10s", 10 * time.Second},
}

for _, test := range tests {
s := test.timespec
parsed, err := ParseOffset(s)
assert.Nil(t, err, "error parsing %s", s)
assert.Equal(t, test.expectedDuration, parsed, "incorrect parsed value for %s", s)
}
}

func TestParseDurationErrors(t *testing.T) {
tests := []string{
"10s",
Expand Down Expand Up @@ -104,3 +129,69 @@ func TestAbsoluteOffset(t *testing.T) {
assert.Equal(t, test.expectedTime, parsed, "incorrect parsed value for %s", s)
}
}

// April 3 2013, 4:05
func TestParseTimeReference(t *testing.T) {
tests := []struct {
ref string
expectedTime time.Time
}{
{"", relativeTo},
{"now", relativeTo},
{"8:50", relativeTo.Add((time.Hour * 4) + (time.Minute * 45))},
{"8:50am", relativeTo.Add((time.Hour * 4) + (time.Minute * 45))},
{"8:50pm", relativeTo.Add((time.Hour * 16) + (time.Minute * 45))},
{"8am", relativeTo.Add((time.Hour * 3) + (time.Minute * 55))},
{"10pm", relativeTo.Add((time.Hour * 17) + (time.Minute * 55))},
{"noon", relativeTo.Add((time.Hour * 7) + (time.Minute * 55))},
{"midnight", relativeTo.Add((time.Hour * -4) + (time.Minute * -5))},
{"teatime", relativeTo.Add((time.Hour * 12) + (time.Minute * -5))},
{"yesterday", relativeTo.Add(time.Hour * 24 * -1)},
{"today", relativeTo},
{"tomorrow", relativeTo.Add(time.Hour * 24)},
{"04/24/13", relativeTo.Add(time.Hour * 24 * 21)},
{"04/24/2013", relativeTo.Add(time.Hour * 24 * 21)},
{"20130424", relativeTo.Add(time.Hour * 24 * 21)},
{"may6", relativeTo.Add(time.Hour * 24 * 33)},
{"may06", relativeTo.Add(time.Hour * 24 * 33)},
{"december17", relativeTo.Add(time.Hour * 24 * 258)},
{"monday", relativeTo.Add(time.Hour * 24 * -2)},
}

for _, test := range tests {
ref := test.ref
parsed, err := ParseTimeReference(ref, relativeTo)
assert.Nil(t, err, "error parsing %s", ref)
assert.Equal(t, test.expectedTime, parsed, "incorrect parsed value for %s", ref)
}
}

func TestParseTimeReferenceErrors(t *testing.T) {
tests := []string{
"january800",
"january",
"random",
":",
}

for _, test := range tests {
parsed, err := ParseTimeReference(test, relativeTo)
assert.Error(t, err)
assert.Equal(t, time.Time{}, parsed)
}
}

func TestParseOffsetErrors(t *testing.T) {
tests := []string{
"something",
"1m",
"10",
"month",
}

for _, test := range tests {
parsed, err := ParseOffset(test)
assert.Error(t, err)
assert.Equal(t, time.Duration(0), parsed)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should have empty endline at end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, any time i add this, it gets removed by go fmt

teddywahle marked this conversation as resolved.
Show resolved Hide resolved