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

*: basic support for builtin function STR_TO_DATE #2078

Merged
merged 12 commits into from Nov 28, 2016

Conversation

tiancaiamao
Copy link
Contributor

  1. add STR_TO_DATE function to support str_to_date('20161122165022','%Y%m%d%H%i%s')
  2. fix bug of strcmp introduced by previous refact

many specifiers in the format string are not supported yet
and more test will be add in next PR

if date == "" {
return nil
}
return errors.New("not enough data in date")
Copy link
Member

Choose a reason for hiding this comment

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

Can this use terror?

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 test mysql behavior and find it never report error, just return NULL on fail.
I'll change to that.

// Just one character.
if len(format) == 1 {
if format[0] == '%' {
return "", "", errors.New("bad format")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@tiancaiamao
Copy link
Contributor Author

@@ -673,6 +675,416 @@ func builtinFromUnixTime(args []types.Datum, _ context.Context) (d types.Datum,
return builtinDateFormat([]types.Datum{d, args[1]}, nil)
}

// strToDate convert date string according to format, return true on success,
Copy link
Member

Choose a reason for hiding this comment

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

converts

Copy link
Member

Choose a reason for hiding this comment

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

This function never meet 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.

builtinDateFormat would return a NULL datum if input date and format don't match, so we don't need return error

)
goTime = mysql.ZeroTime
if !strToDate(&goTime, date, format) {
d.SetNull()
Copy link
Contributor

Choose a reason for hiding this comment

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

This line may be useless, KindNull equals to 0 which is a default value.

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 prefer d.SetNull() here explicitly, it can remind us this function return NULL if strToDate fail...
depend on KindNull == 0 is not that reliable on some scenes

if format[0] == '%' {
return "", "", false
}
return format[:1], format[1:], true
Copy link
Contributor

Choose a reason for hiding this comment

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

Will return format, "", true be more explicit?

@@ -580,3 +580,31 @@ func (s *testEvaluatorSuite) TestDateArith(c *C) {
}
}
}

func (s *testEvaluatorSuite) TestStrToDate(c *C) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test case for %a %b %c %D.

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 would be done in next PR

if len(input) >= 3 {
dayName := input[:3]
if day, ok := weekdayAbbrev[dayName]; ok {
timeSetDay(t, int(day))
Copy link
Member

Choose a reason for hiding this comment

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

WeekDay is not DayOfMonth.

return format[:1], format[1:], true
}

func skipWhiteSpace(input string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we skip the spaces at the end of string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, strToDate will skip any spaces, including the end

return strToDate(t, dateRemain, formatRemain)
}

func getFormatToken(format string) (token string, remain string, succ bool) {
Copy link
Member

Choose a reason for hiding this comment

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

add comments for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?
this is not an exported function and very easy to understand, just as the function name says~

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand the meanings of "token" and "remain" without reading the MySQL manual.

func abbreviatedWeekday(t *time.Time, input string) (string, bool) {
if len(input) >= 3 {
dayName := input[:3]
if _, ok := weekdayAbbrev[dayName]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

Is this case sensitive?

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'll confirm the behavior of MySQL, in the next PRs
Currently this function is not really supported, as our time representation is not compatible with mysql.

goTime time.Time
)
goTime = types.ZeroTime
if !strToDate(&goTime, date, format) {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we define a struct to call strToData, I think it's tooooo ugly to pass a pointer to get result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugly? it's very common in C, and C-family languages. and Go is one of C-family

struct timeData {...} 
var d timeData 
d.strToDate() ??

I think it's ... Poor man's object ...

@@ -673,6 +675,328 @@ func builtinFromUnixTime(args []types.Datum, _ context.Context) (d types.Datum,
return builtinDateFormat([]types.Datum{d, args[1]}, nil)
}

// strToDate converts date string according to format, return true on success,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/return/returns

}

func monthNumeric(t *time.Time, input string) (string, bool) {
// TODO: this code is ugly!
Copy link
Contributor

Choose a reason for hiding this comment

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

s/this/This

if len(input) >= 3 {
dayName := input[:3]
if _, ok := weekdayAbbrev[dayName]; ok {
// TODO: we need refact mysql time to support this
Copy link
Contributor

Choose a reason for hiding this comment

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

s/we/We

@shenli
Copy link
Member

shenli commented Nov 27, 2016

@zimulala @coocood PTAL

@coocood
Copy link
Member

coocood commented Nov 28, 2016

LGTM


func monthNumeric(t *time.Time, input string) (string, bool) {
// TODO: This code is ugly!
for i := 12; i >= 0; i-- {
Copy link
Member

Choose a reason for hiding this comment

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

Why i >= 0, month 0 is valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

}

func dayOfMonthNumericTwoDigits(t *time.Time, input string) (string, bool) {
v, succ := parseTwoDigits(input)
Copy link
Member

Choose a reason for hiding this comment

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

Is 2.31 valid ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mysql behavior:

mysql> select str_to_date('2016 02 31', '%Y %m %D');
+---------------------------------------+
| str_to_date('2016 02 31', '%Y %m %D') |
+---------------------------------------+
| 2016-02-31                            |
+---------------------------------------+
1 row in set (0.00 sec)

mysql permit invalid date, while Go can't...and that why we need refact.
Of course, I'll be glad if you also review this PR #2098

@hanfei1991
Copy link
Member

LGTM

@tiancaiamao tiancaiamao merged commit 3c34925 into master Nov 28, 2016
@tiancaiamao tiancaiamao deleted the tiancaiamao/str-to-date branch November 28, 2016 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants