feat: introduce `java.time` methods and variables by diegomarquezp · Pull Request #2780 · googleapis/java-bigquerystorage · GitHub

This PR introduces java.time alternatives to existing org.threeten.bp.* methods, as well as switching internal variables (if any) to java.time

The main constraint is to keep the changes backwards compatible, so for each existing threeten method "method1(org.threeten.bp.Duration)" we will add an alternative with a Duration (or Timestamp when applicable) suffix: "method1Duration(java.time.Duration)".

For most cases, the implementation will be held in the java.time method and the old threeten method will just delegate the call to it. However, for the case of abstract classes, the implementation will be kept in the threeten method to avoid breaking changes (i.e. users that already overloaded the method in their user code).

@lqiu96 This check (java 8 only) is failing with

Error:    JsonToProtoMessageTest.testTimestampRepeated:890 expected:<test_string_repeated: 10
test_string_t_z_repeated: 1648493279010000
test_long_repeated: 1687984085000000
test_int_repeated: 153480695
test_float_repeated: [15346](https://github.com/googleapis/java-bigquerystorage/actions/runs/11942130315/job/33288467778?pr=2780#step:6:15347)8069500
test_offset_repeated: 1649135171000000
test_timezone_repeated: 1649174771000000
test_saformat_repeated: 1534680660000000
> but was:<test_string_repeated: 10
test_string_t_z_repeated: 1648493279010000
test_long_repeated: 1687984085000000
test_int_repeated: [15348](https://github.com/googleapis/java-bigquerystorage/actions/runs/11942130315/job/33288467778?pr=2780#step:6:15349)0695
test_float_repeated: 153468069500
test_offset_repeated: 1649149571000000
test_timezone_repeated: 1649174771000000
test_saformat_repeated: 1534680660000000
>

The problem is with test_offset_repeated; it's the same as googleapis/sdk-platform-java#3330 (comment), but the difference here is that we use appendOffset() with a default zone string of +00:00, while the one in appendZoneOrOffsetId() is Z

java-bigquerystorage/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/JsonToProtoMessage.java

Lines 95 to 102 in 5f1b30a

    
           .optionalStart() 
        
           .appendOffset("+HH:MM", "+00:00") 
        
           .optionalEnd() 
        
           .optionalStart() 
        
           .appendZoneText(TextStyle.SHORT) 
        
           .optionalEnd() 
        
           .optionalStart() 
        
           .appendLiteral('Z')

This fix works for the current tests. Note that the literal Z is dropped from the config because appendZoneOrOffsetId() uses Z as the no-zone string

Added a test for +00:00 in 260c835

Why is this added in BQStorage and not used from sdk-platform-java?

They are functions only used in BQS so far (confirmed other repos don't need them). However, the reason is simply because it's faster to implement here than waiting for a new release of gax with the utilities we have here. What if we follow up in gax and temporarily have these functions in this repo?

What if we follow up in gax and temporarily have these functions in this repo?

Sure. Can we make this class package-private until we migrate this to Gax? Can you also create an issue in sdk-platform-java for this?

As discussed internally, left as @InternalApi, referencing the new issue in sdk-platform: https://github.com/googleapis/sdk-platform-java/issues/3412

Errors$IsRetryableStatusResult is still marked as public even if it's only used by ApiResultRetryAlgorithm in the BQStorage SDK. I think it's still accessible which is problematic.

@PhongChuong what's the rationale of Errors$IsRetryableStatusResult being public? Is it similar to the case of ApiResultRetryAlgorithm "visible for technical reasons"?
Also, does it make sense to change their datatypes directly, without threeten alternatives as done currently?

I don't know the historical reason as to why it is public but from the code, I would assume that is it "visible for technical reasons". Specifically, while IsRetryableStatusResult is public, it seems to be only used in ApiResultRetryAlgorithm and it is not part of the return value. I think it would be okay to change the datatype directly here.

@yirutang , what do you think?

I am fine making it private. I don't think external user should use it.

Overall the changes looks good. Thank you for the PR, it looks like a lot of work tracking down all the changes needed.

I don't know the historical reason as to why it is public but from the code, I would assume that is it "visible for technical reasons". Specifically, while IsRetryableStatusResult is public, it seems to be only used in ApiResultRetryAlgorithm and it is not part of the return value. I think it would be okay to change the datatype directly here.

@yirutang , what do you think?

LGTM. Added a small comment if you could address

feat: introduce java.time methods and variables

7feaaa8

product-auto-label Bot added size: l Pull request size is large. api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. labels Nov 20, 2024

diegomarquezp added 2 commits November 20, 2024 16:27

fix javadoc

9178b4b

add clirr ignores

83256e7

diegomarquezp marked this pull request as ready for review November 22, 2024 18:41

diegomarquezp requested review from a team, hongalex and lqiu96 November 22, 2024 18:41

use java 8 fix for offset parsing

525d5e1

diegomarquezp commented Nov 22, 2024

View reviewed changes

expand offset test

260c835

lqiu96 reviewed Nov 26, 2024

View reviewed changes

Comment thread google-cloud-bigquerystorage/clirr-ignored-differences.xml

lqiu96 reviewed Nov 26, 2024

View reviewed changes

Comment thread google-cloud-bigquerystorage/src/test/proto/jsonTest.proto

diegomarquezp requested a review from lqiu96 November 27, 2024 22:31

PhongChuong self-requested a review November 28, 2024 11:01

PhongChuong reviewed Nov 28, 2024

View reviewed changes

diegomarquezp requested a review from PhongChuong December 3, 2024 20:46

PhongChuong approved these changes Dec 4, 2024

View reviewed changes

lqiu96 approved these changes Dec 4, 2024

View reviewed changes

diegomarquezp mentioned this pull request Apr 1, 2026

[sdk-platform-java] TimeConversionUtils should include LocalTime and LocalDateTime conversions googleapis/google-cloud-java#12403

Open

makr time conversion utils as internal

2ac7606

diegomarquezp added the automerge Merge the pull request once unit tests and other checks pass. label Dec 4, 2024

Merge branch 'main' into introduce-java-time

9e13c15

gcf-merge-on-green Bot merged commit 8dd66d5 into main Dec 5, 2024

gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 5, 2024

gcf-merge-on-green Bot deleted the introduce-java-time branch December 5, 2024 02:10

release-please Bot mentioned this pull request Dec 5, 2024

chore(main): release 3.11.0 #2781

Merged

release-please Bot mentioned this pull request Dec 15, 2025

chore(protobuf-4.x-rc): release 3.20.0-rc1 #3127

Merged

+                  <to>java.time.Duration</to>
+                </difference>
+                <difference>
+                  <!--The retryDelay field is used by ApiResultRetryAlgorithm, which is marked as @InternalApi-->

+                  <to>java.time.Duration</to>
+                </difference>
+                <difference>
+                  <!--The retryDelay field is used by ApiResultRetryAlgorithm, which is marked as @InternalApi-->


		package com.google.cloud.bigquery.storage.util;

		public class TimeConversionUtils {

feat: introduce `java.time` methods and variables by diegomarquezp · Pull Request #2780 · googleapis/java-bigquerystorage · GitHub

Uh oh!

feat: introduce java.time methods and variables - #2780

feat: introduce java.time methods and variables#2780gcf-merge-on-green[bot] merged 7 commits intomaingoogleapis/java-bigquerystorage:mainfrom introduce-java-timegoogleapis/java-bigquerystorage:introduce-java-timeCopy head branch name to clipboard

Conversation

diegomarquezp commented Nov 20, 2024

Uh oh!

diegomarquezp commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PhongChuong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: introduce java.time methods and variables#2780
gcf-merge-on-green[bot] merged 7 commits into
mainfrom
introduce-java-time

diegomarquezp commented Nov 22, 2024 •
edited
Loading