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

[Typing] Simplify Ext Parse #896

Merged
merged 10 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 13 additions & 48 deletions lib/typing/ext/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package ext

import (
"fmt"
"log/slog"
"time"
)

Expand All @@ -24,35 +23,22 @@ func ParseFromInterface(val any, additionalDateFormats []string) (*ExtendedTime,
}
}

// ParseTimeExactMatch - This function is the same as `ParseTimeExactMatchLegacy` with the only exception that it'll return an error if it was not an exact match
// ParseTimeExactMatch will return an error if it was not an exact match.
// We need this function because things may parse correctly but actually truncate precision
func ParseTimeExactMatch(layout, timeString string) (time.Time, error) {
ts, err := time.Parse(layout, timeString)
func ParseTimeExactMatch(layout, value string) (time.Time, error) {
ts, err := time.Parse(layout, value)
if err != nil {
return time.Time{}, err
}

if ts.Format(layout) != timeString {
return time.Time{}, fmt.Errorf("failed to parse %q with layout %q", timeString, layout)
if ts.Format(layout) != value {
return time.Time{}, fmt.Errorf("failed to parse %q with layout %q", value, layout)
}

return ts, nil
}

// TODO: Remove callers from this.
// ParseTimeExactMatchLegacy is a wrapper around time.Parse() and will return an extra boolean to indicate if it was an exact match or not.
// Parameters: layout, potentialDateTimeString
// Returns: time.Time object, exactLayout (boolean), error
func ParseTimeExactMatchLegacy(layout, potentialDateTimeString string) (time.Time, bool, error) {
ts, err := time.Parse(layout, potentialDateTimeString)
if err != nil {
return ts, false, err
}

return ts, ts.Format(layout) == potentialDateTimeString, nil
}

// ParseExtendedDateTime will take a string and check if the string is of the following types:
// ParseExtendedDateTime will take a string and check if the string is of the following types:
// - Timestamp w/ timezone
// - Timestamp w/o timezone
// - Date
Expand All @@ -62,48 +48,27 @@ func ParseTimeExactMatchLegacy(layout, potentialDateTimeString string) (time.Tim
// 1) Precision loss in translation
// 2) Original format preservation (with tz locale).
// If it cannot find it, then it will give you the next best thing.
func ParseExtendedDateTime(dtString string, additionalDateFormats []string) (*ExtendedTime, error) {
// Check all the timestamp formats
var potentialFormat string
var potentialTime time.Time
func ParseExtendedDateTime(val string, additionalDateFormats []string) (*ExtendedTime, error) {
// TODO: ExtendedTimeKindType so we can selectively parse.
for _, supportedDateTimeLayout := range supportedDateTimeLayouts {
ts, exactMatch, err := ParseTimeExactMatchLegacy(supportedDateTimeLayout, dtString)
if err == nil {
potentialFormat = supportedDateTimeLayout
potentialTime = ts
if exactMatch {
return NewExtendedTime(ts, DateTimeKindType, supportedDateTimeLayout), nil
}
if ts, err := ParseTimeExactMatch(supportedDateTimeLayout, val); err == nil {
return NewExtendedTime(ts, DateTimeKindType, supportedDateTimeLayout), nil
}
}

// Now check DATE formats, btw you can append nil arrays
for _, supportedDateFormat := range append(supportedDateFormats, additionalDateFormats...) {
ts, exactMatch, err := ParseTimeExactMatchLegacy(supportedDateFormat, dtString)
if err == nil && exactMatch {
if ts, err := ParseTimeExactMatch(supportedDateFormat, val); err == nil {
return NewExtendedTime(ts, DateKindType, supportedDateFormat), nil
}
}

// TODO: Remove this if we don't see any Sentry.
// Now check TIME formats
for _, supportedTimeFormat := range SupportedTimeFormatsLegacy {
ts, exactMatch, err := ParseTimeExactMatchLegacy(supportedTimeFormat, dtString)
if err == nil && exactMatch {
slog.Error("Unexpected call to SupportedTimeFormatsLegacy",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this entire loop? Looks like we don't actually expect to match any time formats, but with this change we'll still support matching time formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should keep it because we'll need it for the inference work later (When we encounter a TIME column later and we have a string value, we should try to parse the string value as TIME)

slog.String("dtString", dtString),
slog.String("supportedTimeFormat", supportedTimeFormat),
)
if ts, err := ParseTimeExactMatch(supportedTimeFormat, val); err == nil {
return NewExtendedTime(ts, TimeKindType, supportedTimeFormat), nil
}
}

// If nothing fits, return the next best thing.
if potentialFormat != "" {
// TODO: Remove this if we don't see any logs.
slog.Warn("Failed to find exact match for dtString, returning next best thing", slog.String("dtString", dtString), slog.String("potentialFormat", potentialFormat))
return NewExtendedTime(potentialTime, DateTimeKindType, potentialFormat), nil
}

return nil, fmt.Errorf("dtString: %s is not supported", dtString)
return nil, fmt.Errorf("unsupported value: %q", val)
}
15 changes: 0 additions & 15 deletions lib/typing/ext/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,6 @@ func TestParseExtendedDateTime_Timestamp(t *testing.T) {
assert.Equal(t, "2023-04-24T17:29:05.69944Z", extTime.String(""))
}

func TestParseExtendedDateTime(t *testing.T) {
{
dateString := "27/12/82"
extTime, err := ParseExtendedDateTime(dateString, []string{"02/01/06"})
assert.NoError(t, err)
assert.Equal(t, "27/12/82", extTime.String(""))
}
{
dtString := "Mon Jan 02 15:04:05.69944 -0700 2006"
ts, err := ParseExtendedDateTime(dtString, nil)
assert.NoError(t, err)
assert.NotEqual(t, ts.String(""), dtString)
}
}

func TestTimeLayout(t *testing.T) {
ts := time.Now()

Expand Down
Loading