Skip to content

Conversation

@jycor
Copy link
Contributor

@jycor jycor commented Feb 4, 2025

This PR fixes an issue with the str_to_date function where we wouldn't match string literals in the date with literals in the format, because we were improperly converting them to lowercase.

Additionally, this PR has it so the str_to_date function returns a time.Time instead of a string. This gets us closer to MySQL behavior over the server.

fixes: dolthub/dolt#8807

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple minor comments and then a question about some of the error test queries.

{"no_day", "Jan 2000", "%b %y", "2020-01-00", ""},
{"day_of_month_and_day_of_year", "Jan 3, 100 2000", "%b %e, %j %y", "2020-04-09", ""},
{"no_year", "Jan 3", "%b %e", time.Date(0, time.January, 3, 0, 0, 0, 0, time.UTC), ""},
{"no_day", "Jan 2000", "%b %y", time.Date(2019, time.December, 31, 0, 0, 0, 0, time.UTC), ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

This one confused me... shouldn't we return Jan 2000, instead of Dec 31st, 2019?

When I tested this one on MySQL (version 9.0.1 at least, we may want to follow 8.4's behavior, but I didn't have that running currently), I got NULL for this query and the previous query.

Copy link
Contributor Author

@jycor jycor Feb 5, 2025

Choose a reason for hiding this comment

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

I believe that the test was a incorrect. It should be %Y instead of %y (the test right after is also wrong for the same reason). After that, I think it's some weirdness with the golang time package. There's no 0 day, so it just goes back to the previous year. Additionally, SQL_MODE needs to include NO_ZERO_IN_DATE and NO_ZERO_DATE to have those be NULL, but we don't support either of those in dolt.

Oddly, it seems like time.Date(1999, time.December, 31...) == time.Date(2000, time.January, 0, ...)

@jycor jycor merged commit 6e5593e into main Feb 5, 2025
8 checks passed
@jycor jycor deleted the james/str-date-case branch February 5, 2025 19:09
@jycor jycor changed the title fix case insensitivity for str to date fix case insensitivity and return type for str_to_date Feb 5, 2025
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.

Issue with "T" when using str_to_date

3 participants