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

Fix parsing of timestamp strings with missing fractional seconds; add unit test #185

Merged
merged 2 commits into from
Jun 29, 2016

Conversation

mchalek
Copy link
Contributor

@mchalek mchalek commented Jun 28, 2016

I have encountered a failure due to BigQuery returning timestamps with missing fractional seconds. In these instances, the timestamp parser throws an IllegalArgumentException because the expected pattern is "too short", e.g.:

[info]   java.lang.IllegalArgumentException: Invalid format: "2016-06-28 22:13:29" is too short
[info]   at org.joda.time.format.DateTimeFormatter.parseDateTime(DateTimeFormatter.java:945)
[info]   at com.spotify.scio.bigquery.package$Timestamp$.parse(package.scala:95)
[info]   at com.spotify.scio.bigquery.TimestampTest$$anonfun$3.apply$mcV$sp(TimestampTest.scala:40)
[info]   at com.spotify.scio.bigquery.TimestampTest$$anonfun$3.apply(TimestampTest.scala:36)
[info]   at com.spotify.scio.bigquery.TimestampTest$$anonfun$3.apply(TimestampTest.scala:36)

Note that I received this error when querying tables that I generated using scio-bigquery, with input data that originally did not contain any fractional seconds. I'm not quite sure why BigQuery does not return zeros in the fractional second columns when they are queried, but this revision ensures that the parser does not break when this happens.

I added a unit test for this condition, which will illustrate the failure when run without the changes to package.scala in place.

@codecov-io
Copy link

Current coverage is 74.32%

Merging #185 into master will decrease coverage by <.01%

@@             master       #185   diff @@
==========================================
  Files            62         62          
  Lines          2151       2158     +7   
  Methods        1983       1988     +5   
  Messages          0          0          
  Branches        168        170     +2   
==========================================
+ Hits           1599       1604     +5   
- Misses          552        554     +2   
  Partials          0          0          

Powered by Codecov. Last updated by 406ca51...4c84cfc

@nevillelyh nevillelyh merged commit 804e56d into spotify:master Jun 29, 2016
@nevillelyh
Copy link
Contributor

Thanks for fixing this!

nevillelyh pushed a commit that referenced this pull request Jun 29, 2016
… unit test (#185)

* Fix parsing of timestamp strings with missing fractional seconds; add unit test

* fix silly line length requirement
@mchalek
Copy link
Contributor Author

mchalek commented Jun 29, 2016

@nevillelyh no problem! Thanks for putting this library together, it's already saved me far more time than it took me to make this little patch.

cheers!

kevin

@nevillelyh
Copy link
Contributor

@mchalek good to hear! It'd be great if you can add something to this: https://github.com/spotify/scio/wiki/Powered-By

nevillelyh pushed a commit that referenced this pull request Jun 29, 2016
… unit test (#185)

* Fix parsing of timestamp strings with missing fractional seconds; add unit test

* fix silly line length requirement
@mchalek
Copy link
Contributor Author

mchalek commented Jul 1, 2016

@nevillelyh we're currently in experimentation mode so we're not sure what we'll end up putting into production, but if we do end up using scio I'll try to remember to add us to the list!

nevillelyh pushed a commit that referenced this pull request Oct 21, 2016
… unit test (#185)

* Fix parsing of timestamp strings with missing fractional seconds; add unit test

* fix silly line length requirement
nevillelyh pushed a commit that referenced this pull request Jan 11, 2017
… unit test (#185)

* Fix parsing of timestamp strings with missing fractional seconds; add unit test

* fix silly line length requirement
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.

3 participants