-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: better ISO8601 compliance #1589
Conversation
@geirsagberg with final DateTimeFormatter formatter =
new DateTimeFormatterBuilder()
.parseLenient()
.append(DateTimeFormatter.ISO_LOCAL_DATE)
.optionalStart()
.appendLiteral('T')
.optionalEnd()
.optionalStart()
.appendLiteral(' ')
.optionalEnd()
.appendValue(HOUR_OF_DAY, 2)
.appendLiteral(':')
.appendValue(MINUTE_OF_HOUR, 2)
.optionalStart()
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 2)
.optionalStart()
.appendFraction(NANO_OF_SECOND, 6, 9, true)
.optionalStart()
.appendOffset("+HHMM", "+00:00")
.optionalEnd()
.optionalStart()
.appendLiteral('Z')
.optionalEnd()
.toFormatter()
.withZone(ZoneOffset.UTC); All the below work fine: final var t1 = formatter.parse("2022-02-06 07:24:47.84");
final var t2 = formatter.parse("2022-02-06T07:24:47.84Z");
final var t3 = formatter.parse("2022-02-06T07:24:47.84");
final var t4 = formatter.parse("2022-02-06T07:24:47.8400010"); |
Hi, with this change, do you still need #1582 ? |
Also, could you run |
No, this covers both. Thanks! I thought it best to fix one and then the other. |
This has been done. Apologies. |
Okay thanks for confirming. Closing #1582 as it is no longer needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add unit tests for this also? thanks!
@stephaniewang526 unit tests added. |
Thanks! However, I am observing an integration test failure:
Could you please have a look? |
Yes, this has been fixed. Looks like it was still using the old method of converting the timestamp |
… libraries Signed-off-by: dark0dave <[email protected]>
@stephaniewang526 I have also rebased |
@stephaniewang526 is there any more work required for this pr? |
🤖 I have created a release *beep* *boop* --- ## [2.12.0](v2.11.1...v2.12.0) (2022-04-01) ### Features * Deprecate format specific `row_count` field in Read API ([#1599](#1599)) ([6f415f6](6f415f6)) ### Bug Fixes * better ISO8601 compliance ([#1589](#1589)) ([29fa8b7](29fa8b7)) ### Dependencies * update dependency com.google.cloud:google-cloud-bigquery to v2.10.5 ([#1602](#1602)) ([8787b5d](8787b5d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Using: https://github.com/googleapis/java-bigquery/blob/main/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/QueryParameterValue.java#L79 we can construct something similar here, to allow for better ISO8601 compliance.
Fixes #1579
Fixes #1580