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

diegomarquezp · 2024-11-20T21:18:43Z

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).

diegomarquezp · 2024-11-22T18:41:16Z

@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')

diegomarquezp · 2024-11-22T19:05:41Z

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

          .appendLiteral(' ')
          .optionalEnd()
          .optionalStart()
-          .appendOffset("+HH:MM", "+00:00")
+          .appendZoneOrOffsetId()
          .optionalEnd()
          .optionalStart()
          .appendZoneText(TextStyle.SHORT)
          .optionalEnd()
-          .optionalStart()
-          .appendLiteral('Z')
-          .optionalEnd()
          .toFormatter()
          .withZone(ZoneOffset.UTC);


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

lqiu96 · 2024-11-26T20:00:33Z

...igquerystorage/src/main/java/com/google/cloud/bigquery/storage/util/TimeConversionUtils.java

+
+package com.google.cloud.bigquery.storage.util;
+
+public class TimeConversionUtils {


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: googleapis/sdk-platform-java#3412

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

lqiu96 · 2024-11-26T20:31:31Z

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

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


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.

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

PhongChuong

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

PhongChuong · 2024-11-28T12:34:38Z

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

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


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?

lqiu96

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 as code owners November 22, 2024 18:41

diegomarquezp requested review from 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

google-cloud-bigquerystorage/clirr-ignored-differences.xml Show resolved Hide resolved

lqiu96 reviewed Nov 26, 2024

View reviewed changes

google-cloud-bigquerystorage/src/test/proto/jsonTest.proto Show resolved Hide resolved

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 Dec 4, 2024

TimeConversionUtils should include LocalTime and LocalDateTime conversions googleapis/sdk-platform-java#3412

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
21 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

diegomarquezp commented Nov 20, 2024

diegomarquezp commented Nov 22, 2024 •

edited

Loading

diegomarquezp Nov 22, 2024

diegomarquezp Nov 25, 2024

lqiu96 Nov 26, 2024

diegomarquezp Nov 27, 2024

lqiu96 Dec 4, 2024

diegomarquezp Dec 4, 2024

lqiu96 Nov 26, 2024

diegomarquezp Nov 27, 2024

PhongChuong Nov 28, 2024

yirutang Dec 3, 2024

PhongChuong left a comment

PhongChuong Nov 28, 2024

lqiu96 left a comment


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

		public class TimeConversionUtils {

feat: introduce java.time methods and variables #2780

feat: introduce java.time methods and variables #2780

Conversation

diegomarquezp commented Nov 20, 2024

diegomarquezp commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhongChuong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 left a comment

Choose a reason for hiding this comment

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

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

diegomarquezp commented Nov 22, 2024 •

edited

Loading