Skip to content
This repository was archived by the owner on Feb 24, 2026. It is now read-only.

fix: better ISO8601 compliance#1589

Merged
stephaniewang526 merged 2 commits intogoogleapis:mainfrom
dark0dave:fix/ISO8601
Mar 31, 2022
Merged

fix: better ISO8601 compliance#1589
stephaniewang526 merged 2 commits intogoogleapis:mainfrom
dark0dave:fix/ISO8601

Conversation

@dark0dave
Copy link
Copy Markdown
Contributor

@dark0dave dark0dave commented Mar 26, 2022 •
edited by stephaniewang526
Loading

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

@dark0dave dark0dave requested review from a team and shollyman March 26, 2022 15:28
@product-auto-label product-auto-label bot added the api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. label Mar 26, 2022
@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 26, 2022 •
edited
Loading

@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");

@dark0dave dark0dave changed the title fix: better ISO8601 fix: better ISO8601 compliance Mar 26, 2022
@stephaniewang526
Copy link
Copy Markdown
Contributor

stephaniewang526 commented Mar 28, 2022 •
edited
Loading

Hi, with this change, do you still need #1582 ?

@stephaniewang526
Copy link
Copy Markdown
Contributor

stephaniewang526 commented Mar 28, 2022

Also, could you run mvn com.coveo:fmt-maven-plugin:format to fix the lint issues?

@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 28, 2022

Hi, with this change, do you still need #1582 ?

No, this covers both. Thanks! I thought it best to fix one and then the other.

@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 28, 2022

Also, could you run mvn com.coveo:fmt-maven-plugin:format to fix the lint issues?

This has been done. Apologies.

@stephaniewang526
Copy link
Copy Markdown
Contributor

stephaniewang526 commented Mar 28, 2022

Okay thanks for confirming. Closing #1582 as it is no longer needed.

@stephaniewang526 stephaniewang526 mentioned this pull request Mar 28, 2022
Closed
@stephaniewang526 stephaniewang526 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 28, 2022
stephaniewang526
stephaniewang526 suggested changes Mar 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@stephaniewang526 stephaniewang526 left a 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!

@dark0dave dark0dave requested review from a team and stephaniewang526 March 28, 2022 19:08
@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 28, 2022

@stephaniewang526 unit tests added.

@stephaniewang526 stephaniewang526 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 29, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 29, 2022
@stephaniewang526 stephaniewang526 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Mar 29, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 29, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 29, 2022
@stephaniewang526
Copy link
Copy Markdown
Contributor

stephaniewang526 commented Mar 29, 2022

@stephaniewang526 unit tests added.

Thanks! However, I am observing an integration test failure:

java.lang.AssertionError: expected:<1644132287840> but was:<1644132287840000>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at com.google.cloud.bigquery.storage.v1.it.ITBigQueryWriteManualClientTest.testJsonStreamWriterWithDefaultStream(ITBigQueryWriteManualClientTest.java:459)

Could you please have a look?

@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 30, 2022

@stephaniewang526 unit tests added.

Thanks! However, I am observing an integration test failure:

java.lang.AssertionError: expected:<1644132287840> but was:<1644132287840000>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at com.google.cloud.bigquery.storage.v1.it.ITBigQueryWriteManualClientTest.testJsonStreamWriterWithDefaultStream(ITBigQueryWriteManualClientTest.java:459)

Could you please have a look?

Yes, this has been fixed. Looks like it was still using the old method of converting the timestamp

@dark0dave
fix: Timestamps parsed using datetimeformater similar to other google…
72b30a5 
… libraries

Signed-off-by: dark0dave <dark0dave@mykolab.com>
@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 30, 2022

@stephaniewang526 I have also rebased

@stephaniewang526 stephaniewang526 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 30, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 30, 2022
@dark0dave
Copy link
Copy Markdown
Contributor Author

dark0dave commented Mar 31, 2022 •
edited
Loading

@stephaniewang526 is there any more work required for this pr?

@stephaniewang526 stephaniewang526 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 31, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 31, 2022
@stephaniewang526 stephaniewang526 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2022
@stephaniewang526 stephaniewang526 merged commit 29fa8b7 into googleapis:main Mar 31, 2022
@release-please release-please bot mentioned this pull request Mar 31, 2022
Merged
@dark0dave dark0dave mentioned this pull request Apr 2, 2022
Closed
gcf-merge-on-green bot pushed a commit that referenced this pull request Apr 4, 2022
@release-please
chore(main): release 2.12.0 (#1600)
18d5935 
🤖 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).
@dark0dave dark0dave deleted the fix/ISO8601 branch March 31, 2023 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@shollyman shollyman Awaiting requested review from shollyman

1 more reviewer

@stephaniewang526 stephaniewang526 stephaniewang526 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BigQuery Storage Write API: ISO 8601 timestamp not supported in JsonStreamWriter Timestamp still not parsed correctly

3 participants

@dark0dave @stephaniewang526 @yoshi-kokoro