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

[Date] Implement %y year without century parser / formatter; handle incomplete digits #8

Merged
merged 5 commits into from
Jan 29, 2024

Conversation

ohaibbq
Copy link

@ohaibbq ohaibbq commented Jan 26, 2024

Closes Recidiviz/recidiviz-data#20755

From the docs, %y is as follows:

The year without century as a decimal number (00-99), with an optional leading zero. Can be mixed with %C. If %C is not specified, years 00-68 are 2000s, while years 69-99 are 1900s.

The note about %C in the documentation refers to how to interpret the results of %y rather than saying that it is an alias of the element

}

func yearWithoutCenturyFormatter(t *time.Time) ([]rune, error) {
year := t.Format("2006")

Choose a reason for hiding this comment

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

Why is 2006 hard-coded here?

Copy link
Author

Choose a reason for hiding this comment

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

@ageiduschek Golang uses a hardcoded datetime for its parse / format string rather than %X etc

https://go.dev/src/time/format.go

// Here is a summary of the components of a layout string. Each element shows by
// example the formatting of an element of the reference time. Only these values
// are recognized. Text in the layout string that is not recognized as part of
// the reference time is echoed verbatim during Format and expected to appear
// verbatim in the input to Parse.
//
//	Year: "2006" "06"
//	Month: "Jan" "January" "01" "1"
//	Day of the week: "Mon" "Monday"
//	Day of the month: "2" "_2" "02"
//	Day of the year: "__2" "002"
//	Hour: "15" "3" "03" (PM or AM)
//	Minute: "4" "04"
//	Second: "5" "05"
//	AM/PM mark: "PM"

RFC3339 = "2006-01-02T15:04:05Z07:00"

@@ -518,6 +518,40 @@ func centuryFormatter(t *time.Time) ([]rune, error) {
return []rune(fmt.Sprint(t.Year())[:2]), nil
}

func yearWithoutCenturyParser(text []rune, t *time.Time) (int, error) {
const yearLen = 2
if len(text) > yearLen {
Copy link
Author

Choose a reason for hiding this comment

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

This clause is incorrect for the case of 01-MM-DD. I will need to update this.

A related issue is Recidiviz/recidiviz-data#20756

BigQuery generally allows non-zero leading dates, but the emulator does not.

Copy link
Author

Choose a reason for hiding this comment

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

This has been updated to fix goccy#137

@ohaibbq ohaibbq changed the title [Date] Implement %y year without century parser / formatter [Date] Implement %y year without century parser / formatter; handle incomplete digits Jan 26, 2024
@ohaibbq ohaibbq requested a review from ageiduschek January 26, 2024 23:14
const monthLen = 2
if len(text) < monthLen {
return 0, fmt.Errorf("unexpected month number")
func parseDigitRespectingOptionalPlaces(text []rune, minNumber int64, maxNumber int64) (int, int64, error) {
Copy link
Author

Choose a reason for hiding this comment

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

I'd love a closer look at this function. All test cases seem to show it working well, but I'm not sure how understandable it is! @ageiduschek @colincadams

Choose a reason for hiding this comment

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

Oh whoops approved before I saw this comment. I think it could be useful to add a docstring at the beginning of the function to explain what an "optional place" is and give a few examples.

@ohaibbq ohaibbq merged commit ed799e3 into candidate/rb20240116 Jan 29, 2024
3 of 5 checks passed
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.

2 participants