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

feat: Add CivilTimeEncoder to encode and decode DateTime/Time as numerics #937

Merged
merged 27 commits into from Mar 18, 2021

Conversation

JacobStocklass
Copy link
Contributor

@JacobStocklass JacobStocklass commented Mar 11, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@JacobStocklass JacobStocklass requested a review from yirutang Mar 11, 2021
@JacobStocklass JacobStocklass requested a review from as a code owner Mar 11, 2021
@JacobStocklass JacobStocklass requested a review from tswast Mar 11, 2021
@product-auto-label product-auto-label bot added the api: bigquerystorage label Mar 11, 2021
@google-cla google-cla bot added the cla: yes label Mar 11, 2021
@JacobStocklass JacobStocklass requested a review from as a code owner Mar 12, 2021
@JacobStocklass JacobStocklass changed the title feat(Java) Adding Time Encoding Integration Test Placeholder feat(Java): Adding Time Encoding Integration Test Placeholder Mar 12, 2021
@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 12, 2021

Java LocalTime and LocalDateTime use Java 8 so will need to be reverted back to using Joda time so as not to change the language level.

@codecov
Copy link

@codecov codecov bot commented Mar 13, 2021

Codecov Report

Merging #937 (317c773) into master (814c7dd) will decrease coverage by 0.28%.
The diff coverage is 77.52%.

Current head 317c773 differs from pull request most recent head e25aee4. Consider uploading reports for the commit e25aee4 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #937      +/-   ##
============================================
- Coverage     81.14%   80.86%   -0.29%     
- Complexity      996     1038      +42     
============================================
  Files            74       77       +3     
  Lines          5347     5645     +298     
  Branches        415      436      +21     
============================================
+ Hits           4339     4565     +226     
- Misses          838      897      +59     
- Partials        170      183      +13     
Impacted Files Coverage Δ Complexity Δ
...oud/bigquery/storage/v1beta2/CivilTimeEncoder.java 77.52% <77.52%> (ø) 18.00 <18.00> (?)
.../google/cloud/bigquery/storage/v1beta2/Waiter.java 56.04% <0.00%> (-16.90%) 12.00% <0.00%> (-6.00%)
.../cloud/bigquery/storage/v1alpha2/StreamWriter.java 80.39% <0.00%> (-3.93%) 34.00% <0.00%> (-3.00%)
...e/cloud/bigquery/storage/v1beta2/StreamWriter.java 82.33% <0.00%> (-2.70%) 32.00% <0.00%> (-6.00%)
...oud/bigquery/storage/v1beta2/JsonStreamWriter.java 76.52% <0.00%> (-2.47%) 14.00% <0.00%> (-1.00%)
...google/cloud/bigquery/storage/v1alpha2/Waiter.java 55.95% <0.00%> (-1.20%) 12.00% <0.00%> (-2.00%)
...bigquery/storage/v1/stub/GrpcBigQueryReadStub.java 89.53% <0.00%> (ø) 10.00% <0.00%> (ø%)
...ery/storage/v1beta2/stub/GrpcBigQueryReadStub.java 89.53% <0.00%> (ø) 10.00% <0.00%> (ø%)
...ry/storage/v1beta2/stub/GrpcBigQueryWriteStub.java 93.24% <0.00%> (ø) 15.00% <0.00%> (ø%)
...y/storage/v1alpha2/stub/GrpcBigQueryWriteStub.java 93.50% <0.00%> (ø) 15.00% <0.00%> (ø%)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 814c7dd...e25aee4. Read the comment docs.

@JacobStocklass JacobStocklass requested a review from yirutang Mar 15, 2021
@stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Mar 15, 2021

Not a feat, please rename (both subject and initial commit) this PR to test:

@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 15, 2021

Not a feat, please rename (both subject and initial commit) this PR to test:

not sure how I misnamed this so badly. This should be adding in the feature CivilTimeEncoder, I will rename the subject and the initial commit, but should I still use test instead of feat?

@JacobStocklass JacobStocklass changed the title feat(Java): Adding Time Encoding Integration Test Placeholder feat(Java): Add CivilTimeEncoder to encode and decode DateTime/Time as numerics Mar 15, 2021
@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 15, 2021

@yirutang PTAL

@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 16, 2021

@yirutang PTAL. I've removed the e2e test for another PR where we can change the mapping function as well.

Copy link
Contributor

@yirutang yirutang left a comment

Almost there.

@yirutang
Copy link
Contributor

@yirutang yirutang commented Mar 16, 2021

@yirutang
Copy link
Contributor

@yirutang yirutang commented Mar 16, 2021

@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 16, 2021

@yirutang I've made the tweeks to the test code and I think it reads a bit easier now

@JacobStocklass
Copy link
Contributor Author

@JacobStocklass JacobStocklass commented Mar 16, 2021

@stephaniewang526 Hey Stephanie are you able to merge this PR? I believe it is ready

@yirutang yirutang mentioned this pull request Mar 17, 2021
4 tasks
@JacobStocklass JacobStocklass changed the title feat(Java): Add CivilTimeEncoder to encode and decode DateTime/Time as numerics feat: Add CivilTimeEncoder to encode and decode DateTime/Time as numerics Mar 17, 2021
@stephaniewang526 stephaniewang526 merged commit 969b429 into googleapis:master Mar 18, 2021
17 of 18 checks passed
@JacobStocklass JacobStocklass deleted the jstocklass-bug-fix branch Mar 23, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 25, 2021
🤖 I have created a release \*beep\* \*boop\*
---
## [1.16.0](https://www.github.com/googleapis/java-bigquerystorage/compare/v1.15.1...v1.16.0) (2021-03-25)


### Features

* Add CivilTimeEncoder to encode and decode DateTime/Time as numerics ([#937](https://www.github.com/googleapis/java-bigquerystorage/issues/937)) ([969b429](https://www.github.com/googleapis/java-bigquerystorage/commit/969b4290b9934b94b1a0113e04e37ff44b2a536e))


### Bug Fixes

* add a deprecation message on StreamWriter ([#922](https://www.github.com/googleapis/java-bigquerystorage/issues/922)) ([fce5289](https://www.github.com/googleapis/java-bigquerystorage/commit/fce52890c6948a9b78a62d2fe0e4f9768d10d401))


### Dependencies

* update dependency com.google.cloud:google-cloud-bigquery to v1.127.10 ([#955](https://www.github.com/googleapis/java-bigquerystorage/issues/955)) ([c810c72](https://www.github.com/googleapis/java-bigquerystorage/commit/c810c7279bfbad31cb0f94f5ad5d4a74342d4481))
* update dependency com.google.cloud:google-cloud-bigquery to v1.127.9 ([#947](https://www.github.com/googleapis/java-bigquerystorage/issues/947)) ([d781dc5](https://www.github.com/googleapis/java-bigquerystorage/commit/d781dc5479602fee01eb971033978317e5669694))


### Documentation

* **samples:** Check for error from BatchCommitWriteStreams ([#940](https://www.github.com/googleapis/java-bigquerystorage/issues/940)) ([ab3c145](https://www.github.com/googleapis/java-bigquerystorage/commit/ab3c1453d3c1fb627e773d0e7ca4ec991f8d38b7))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants