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: introduce java.time methods and variables #2780

Merged
merged 7 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions google-cloud-bigquerystorage/clirr-ignored-differences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -212,5 +212,21 @@
<className>com/google/cloud/bigquery/storage/v1/StreamWriter</className>
<method>void setMissingValueInterpretationMap(java.util.Map)</method>
</difference>
<difference>
<!--This class is marked as @InternalApi "public for technical reasons"-->
<differenceType>6004</differenceType>
<className>com/google/cloud/bigquery/storage/*/stub/readrows/ApiResultRetryAlgorithm</className>
<field>DEADLINE_SLEEP_DURATION</field>
<from>org.threeten.bp.Duration</from>
<to>java.time.Duration</to>
</difference>
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
<difference>
<!--The retryDelay field is used by ApiResultRetryAlgorithm, which is marked as @InternalApi-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<differenceType>6004</differenceType>
<className>com/google/cloud/bigquery/storage/util/Errors$IsRetryableStatusResult</className>
<field>retryDelay</field>
<from>org.threeten.bp.Duration</from>
<to>java.time.Duration</to>
</difference>
</differences>

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import io.grpc.Metadata;
import io.grpc.Status;
import io.grpc.protobuf.ProtoUtils;
import org.threeten.bp.Duration;
import java.time.Duration;

/** Static utility methods for working with Errors returned from the service. */
public class Errors {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright 2024 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

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

public class TimeConversionUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed internally, left as @InternalApi, referencing the new issue in sdk-platform: googleapis/sdk-platform-java#3412

public static java.time.LocalDateTime toJavaTimeLocalDateTime(
org.threeten.bp.LocalDateTime result) {
return java.time.LocalDateTime.of(
result.getYear(),
java.time.Month.of(result.getMonth().getValue()),
result.getDayOfMonth(),
result.getHour(),
result.getMinute(),
result.getSecond(),
result.getNano());
}

public static org.threeten.bp.LocalDateTime toThreetenLocalDateTime(
java.time.LocalDateTime result) {
return org.threeten.bp.LocalDateTime.of(
result.getYear(),
org.threeten.bp.Month.of(result.getMonth().getValue()),
result.getDayOfMonth(),
result.getHour(),
result.getMinute(),
result.getSecond(),
result.getNano());
}

public static java.time.LocalTime toJavaTimeLocalTime(org.threeten.bp.LocalTime result) {
return java.time.LocalTime.of(
result.getHour(), result.getMinute(), result.getSecond(), result.getNano());
}

public static org.threeten.bp.LocalTime toThreetenLocalTime(java.time.LocalTime result) {
return org.threeten.bp.LocalTime.of(
result.getHour(), result.getMinute(), result.getSecond(), result.getNano());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
*/
package com.google.cloud.bigquery.storage.v1;

import static com.google.cloud.bigquery.storage.util.TimeConversionUtils.toJavaTimeLocalDateTime;
import static com.google.cloud.bigquery.storage.util.TimeConversionUtils.toJavaTimeLocalTime;
import static com.google.cloud.bigquery.storage.util.TimeConversionUtils.toThreetenLocalDateTime;
import static com.google.cloud.bigquery.storage.util.TimeConversionUtils.toThreetenLocalTime;
import static com.google.common.base.Preconditions.checkArgument;

import org.threeten.bp.DateTimeException;
import org.threeten.bp.LocalDateTime;
import org.threeten.bp.LocalTime;
import org.threeten.bp.temporal.ChronoUnit;
import com.google.api.core.ObsoleteApi;
import java.time.DateTimeException;
import java.time.temporal.ChronoUnit;

/**
* Ported from ZetaSQL CivilTimeEncoder Original code can be found at:
Expand Down Expand Up @@ -89,7 +92,7 @@ public final class CivilTimeEncoder {
* @see #decodePacked32TimeSeconds(int)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
private static int encodePacked32TimeSeconds(LocalTime time) {
private static int encodePacked32TimeSeconds(java.time.LocalTime time) {
checkValidTimeSeconds(time);
int bitFieldTimeSeconds = 0x0;
bitFieldTimeSeconds |= time.getHour() << HOUR_SHIFT;
Expand All @@ -112,19 +115,29 @@ private static int encodePacked32TimeSeconds(LocalTime time) {
* @see #encodePacked32TimeSeconds(LocalTime)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
private static LocalTime decodePacked32TimeSeconds(int bitFieldTimeSeconds) {
private static java.time.LocalTime decodePacked32TimeSeconds(int bitFieldTimeSeconds) {
checkValidBitField(bitFieldTimeSeconds, TIME_SECONDS_MASK);
int hourOfDay = getFieldFromBitField(bitFieldTimeSeconds, HOUR_MASK, HOUR_SHIFT);
int minuteOfHour = getFieldFromBitField(bitFieldTimeSeconds, MINUTE_MASK, MINUTE_SHIFT);
int secondOfMinute = getFieldFromBitField(bitFieldTimeSeconds, SECOND_MASK, SECOND_SHIFT);
// LocalTime validates the input parameters.
try {
return LocalTime.of(hourOfDay, minuteOfHour, secondOfMinute);
return java.time.LocalTime.of(hourOfDay, minuteOfHour, secondOfMinute);
} catch (DateTimeException e) {
throw new IllegalArgumentException(e.getMessage(), e);
}
}

/**
* This method is obsolete. Use {@link #encodePacked64TimeMicrosLocalTime(java.time.LocalTime)}
* instead.
*/
@ObsoleteApi("Use encodePacked64TimeMicrosLocalTime(java.time.LocalTime) instead")
@SuppressWarnings("GoodTime")
public static long encodePacked64TimeMicros(org.threeten.bp.LocalTime time) {
return encodePacked64TimeMicrosLocalTime(toJavaTimeLocalTime(time));
}

/**
* Encodes {@code time} as a 8-byte integer with microseconds precision.
*
Expand All @@ -140,13 +153,21 @@ private static LocalTime decodePacked32TimeSeconds(int bitFieldTimeSeconds) {
* @see #encodePacked64TimeMicros(LocalTime)
*/
@SuppressWarnings("GoodTime")
public static long encodePacked64TimeMicros(LocalTime time) {
public static long encodePacked64TimeMicrosLocalTime(java.time.LocalTime time) {
checkValidTimeMicros(time);
return (((long) encodePacked32TimeSeconds(time)) << MICRO_LENGTH) | (time.getNano() / 1_000L);
}

/** This method is obsolete. Use {@link #decodePacked64TimeMicrosLocalTime(long)} instead. */
@ObsoleteApi("Use decodePacked64TimeMicrosLocalTime(long) instead")
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
public static org.threeten.bp.LocalTime decodePacked64TimeMicros(long bitFieldTimeMicros) {
return toThreetenLocalTime(decodePacked64TimeMicrosLocalTime(bitFieldTimeMicros));
}

/**
* Decodes {@code bitFieldTimeMicros} as a {@link LocalTime} with microseconds precision.
* Decodes {@code bitFieldTimeMicros} as a {@link java.time.LocalTime} with microseconds
* precision.
*
* <p>Encoding is as the following:
*
Expand All @@ -159,13 +180,13 @@ public static long encodePacked64TimeMicros(LocalTime time) {
* @see #encodePacked64TimeMicros(LocalTime)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
public static LocalTime decodePacked64TimeMicros(long bitFieldTimeMicros) {
public static java.time.LocalTime decodePacked64TimeMicrosLocalTime(long bitFieldTimeMicros) {
checkValidBitField(bitFieldTimeMicros, TIME_MICROS_MASK);
int bitFieldTimeSeconds = (int) (bitFieldTimeMicros >> MICRO_LENGTH);
LocalTime timeSeconds = decodePacked32TimeSeconds(bitFieldTimeSeconds);
java.time.LocalTime timeSeconds = decodePacked32TimeSeconds(bitFieldTimeSeconds);
int microOfSecond = getFieldFromBitField(bitFieldTimeMicros, MICRO_MASK, MICRO_SHIFT);
checkValidMicroOfSecond(microOfSecond);
LocalTime time = timeSeconds.withNano(microOfSecond * 1000);
java.time.LocalTime time = timeSeconds.withNano(microOfSecond * 1000);
checkValidTimeMicros(time);
return time;
}
Expand All @@ -184,7 +205,7 @@ public static LocalTime decodePacked64TimeMicros(long bitFieldTimeMicros) {
* @see #decodePacked64DatetimeSeconds(long)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
private static long encodePacked64DatetimeSeconds(LocalDateTime dateTime) {
private static long encodePacked64DatetimeSeconds(java.time.LocalDateTime dateTime) {
checkValidDateTimeSeconds(dateTime);
long bitFieldDatetimeSeconds = 0x0L;
bitFieldDatetimeSeconds |= (long) dateTime.getYear() << YEAR_SHIFT;
Expand All @@ -208,16 +229,17 @@ private static long encodePacked64DatetimeSeconds(LocalDateTime dateTime) {
* @see #encodePacked64DatetimeSeconds(LocalDateTime)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
private static LocalDateTime decodePacked64DatetimeSeconds(long bitFieldDatetimeSeconds) {
private static java.time.LocalDateTime decodePacked64DatetimeSeconds(
long bitFieldDatetimeSeconds) {
checkValidBitField(bitFieldDatetimeSeconds, DATETIME_SECONDS_MASK);
int bitFieldTimeSeconds = (int) (bitFieldDatetimeSeconds & TIME_SECONDS_MASK);
LocalTime timeSeconds = decodePacked32TimeSeconds(bitFieldTimeSeconds);
java.time.LocalTime timeSeconds = decodePacked32TimeSeconds(bitFieldTimeSeconds);
int year = getFieldFromBitField(bitFieldDatetimeSeconds, YEAR_MASK, YEAR_SHIFT);
int monthOfYear = getFieldFromBitField(bitFieldDatetimeSeconds, MONTH_MASK, MONTH_SHIFT);
int dayOfMonth = getFieldFromBitField(bitFieldDatetimeSeconds, DAY_MASK, DAY_SHIFT);
try {
LocalDateTime dateTime =
LocalDateTime.of(
java.time.LocalDateTime dateTime =
java.time.LocalDateTime.of(
year,
monthOfYear,
dayOfMonth,
Expand All @@ -231,6 +253,16 @@ private static LocalDateTime decodePacked64DatetimeSeconds(long bitFieldDatetime
}
}

/**
* This method is obsolete. Use {@link
* #encodePacked64DatetimeMicrosLocalDateTime(java.time.LocalDateTime)} instead.
*/
@ObsoleteApi("Use encodePacked64DatetimeMicrosLocalDateTime(java.time.LocalDateTime) instead")
@SuppressWarnings({"GoodTime-ApiWithNumericTimeUnit", "JavaLocalDateTimeGetNano"})
public static long encodePacked64DatetimeMicros(org.threeten.bp.LocalDateTime dateTime) {
return encodePacked64DatetimeMicrosLocalDateTime(toJavaTimeLocalDateTime(dateTime));
}

/**
* Encodes {@code dateTime} as a 8-byte integer with microseconds precision.
*
Expand All @@ -245,14 +277,26 @@ private static LocalDateTime decodePacked64DatetimeSeconds(long bitFieldDatetime
* @see #decodePacked64DatetimeMicros(long)
*/
@SuppressWarnings({"GoodTime-ApiWithNumericTimeUnit", "JavaLocalDateTimeGetNano"})
public static long encodePacked64DatetimeMicros(LocalDateTime dateTime) {
public static long encodePacked64DatetimeMicrosLocalDateTime(java.time.LocalDateTime dateTime) {
checkValidDateTimeMicros(dateTime);
return (encodePacked64DatetimeSeconds(dateTime) << MICRO_LENGTH)
| (dateTime.getNano() / 1_000L);
}

/**
* Decodes {@code bitFieldDatetimeMicros} as a {@link LocalDateTime} with microseconds precision.
* This method is obsolete. Use {@link #decodePacked64DatetimeMicrosLocalDateTime(long)} instead.
*/
@ObsoleteApi("Use decodePacked64DatetimeMicrosLocalDateTime(long) instead")
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
public static org.threeten.bp.LocalDateTime decodePacked64DatetimeMicros(
long bitFieldDatetimeMicros) {
return toThreetenLocalDateTime(
decodePacked64DatetimeMicrosLocalDateTime(bitFieldDatetimeMicros));
}

/**
* Decodes {@code bitFieldDatetimeMicros} as a {@link java.time.LocalDateTime} with microseconds
* precision.
*
* <p>Encoding is as the following:
*
Expand All @@ -265,13 +309,15 @@ public static long encodePacked64DatetimeMicros(LocalDateTime dateTime) {
* @see #encodePacked64DatetimeMicros(LocalDateTime)
*/
@SuppressWarnings("GoodTime-ApiWithNumericTimeUnit")
public static LocalDateTime decodePacked64DatetimeMicros(long bitFieldDatetimeMicros) {
public static java.time.LocalDateTime decodePacked64DatetimeMicrosLocalDateTime(
long bitFieldDatetimeMicros) {
checkValidBitField(bitFieldDatetimeMicros, DATETIME_MICROS_MASK);
long bitFieldDatetimeSeconds = bitFieldDatetimeMicros >> MICRO_LENGTH;
LocalDateTime dateTimeSeconds = decodePacked64DatetimeSeconds(bitFieldDatetimeSeconds);
java.time.LocalDateTime dateTimeSeconds =
decodePacked64DatetimeSeconds(bitFieldDatetimeSeconds);
int microOfSecond = getFieldFromBitField(bitFieldDatetimeMicros, MICRO_MASK, MICRO_SHIFT);
checkValidMicroOfSecond(microOfSecond);
LocalDateTime dateTime = dateTimeSeconds.withNano(microOfSecond * 1_000);
java.time.LocalDateTime dateTime = dateTimeSeconds.withNano(microOfSecond * 1_000);
checkValidDateTimeMicros(dateTime);
return dateTime;
}
Expand All @@ -280,25 +326,25 @@ private static int getFieldFromBitField(long bitField, long mask, int shift) {
return (int) ((bitField & mask) >> shift);
}

private static void checkValidTimeSeconds(LocalTime time) {
private static void checkValidTimeSeconds(java.time.LocalTime time) {
checkArgument(time.getHour() >= 0 && time.getHour() <= 23);
checkArgument(time.getMinute() >= 0 && time.getMinute() <= 59);
checkArgument(time.getSecond() >= 0 && time.getSecond() <= 59);
}

private static void checkValidDateTimeSeconds(LocalDateTime dateTime) {
private static void checkValidDateTimeSeconds(java.time.LocalDateTime dateTime) {
checkArgument(dateTime.getYear() >= 1 && dateTime.getYear() <= 9999);
checkArgument(dateTime.getMonthValue() >= 1 && dateTime.getMonthValue() <= 12);
checkArgument(dateTime.getDayOfMonth() >= 1 && dateTime.getDayOfMonth() <= 31);
checkValidTimeSeconds(dateTime.toLocalTime());
}

private static void checkValidTimeMicros(LocalTime time) {
private static void checkValidTimeMicros(java.time.LocalTime time) {
checkValidTimeSeconds(time);
checkArgument(time.equals(time.truncatedTo(ChronoUnit.MICROS)));
}

private static void checkValidDateTimeMicros(LocalDateTime dateTime) {
private static void checkValidDateTimeMicros(java.time.LocalDateTime dateTime) {
checkValidDateTimeSeconds(dateTime);
checkArgument(dateTime.equals(dateTime.truncatedTo(ChronoUnit.MICROS)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,21 @@
import java.math.BigDecimal;
import java.math.RoundingMode;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.TextStyle;
import java.time.temporal.ChronoField;
import java.time.temporal.TemporalAccessor;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;
import org.threeten.bp.LocalDateTime;
import org.threeten.bp.LocalTime;
import org.threeten.bp.ZoneOffset;
import org.threeten.bp.format.DateTimeFormatter;
import org.threeten.bp.format.DateTimeFormatterBuilder;
import org.threeten.bp.format.TextStyle;
import org.threeten.bp.temporal.ChronoField;
import org.threeten.bp.temporal.TemporalAccessor;

/**
* Converts JSON data to Protobuf messages given the Protobuf descriptor and BigQuery table schema.
Expand Down Expand Up @@ -93,14 +93,11 @@ public class JsonToProtoMessage implements ToProtoConverter<Object> {
.appendLiteral(' ')
.optionalEnd()
.optionalStart()
.appendOffset("+HH:MM", "+00:00")
.appendZoneOrOffsetId()
.optionalEnd()
.optionalStart()
.appendZoneText(TextStyle.SHORT)
.optionalEnd()
.optionalStart()
.appendLiteral('Z')
.optionalEnd()
.toFormatter()
.withZone(ZoneOffset.UTC);
Comment on lines 93 to 102
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test for +00:00 in 260c835


Expand Down Expand Up @@ -604,7 +601,7 @@ private void fillField(
if (val instanceof String) {
protoMsg.setField(
fieldDescriptor,
CivilTimeEncoder.encodePacked64DatetimeMicros(
CivilTimeEncoder.encodePacked64DatetimeMicrosLocalDateTime(
LocalDateTime.parse((String) val, DATETIME_FORMATTER)));
return;
} else if (val instanceof Long) {
Expand All @@ -615,7 +612,8 @@ private void fillField(
if (val instanceof String) {
protoMsg.setField(
fieldDescriptor,
CivilTimeEncoder.encodePacked64TimeMicros(LocalTime.parse((String) val)));
CivilTimeEncoder.encodePacked64TimeMicrosLocalTime(
LocalTime.parse((String) val)));
return;
} else if (val instanceof Long) {
protoMsg.setField(fieldDescriptor, val);
Expand Down Expand Up @@ -872,7 +870,7 @@ private void fillRepeatedField(
if (val instanceof String) {
protoMsg.addRepeatedField(
fieldDescriptor,
CivilTimeEncoder.encodePacked64DatetimeMicros(
CivilTimeEncoder.encodePacked64DatetimeMicrosLocalDateTime(
LocalDateTime.parse((String) val, DATETIME_FORMATTER)));
} else if (val instanceof Long) {
protoMsg.addRepeatedField(fieldDescriptor, val);
Expand All @@ -883,7 +881,8 @@ private void fillRepeatedField(
if (val instanceof String) {
protoMsg.addRepeatedField(
fieldDescriptor,
CivilTimeEncoder.encodePacked64TimeMicros(LocalTime.parse((String) val)));
CivilTimeEncoder.encodePacked64TimeMicrosLocalTime(
LocalTime.parse((String) val)));
} else if (val instanceof Long) {
protoMsg.addRepeatedField(fieldDescriptor, val);
} else {
Expand Down
Loading
Loading