Skip to content

Commit f3b3800

Browse files
lqiu96lqiu96
committed
chore: Address PR feedback
1 parent 5134ab7 commit f3b3800

File tree

3 files changed

+62
-48
lines changed

3 files changed

+62
-48
lines changed

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

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,18 @@
3131
import java.util.HashMap;
3232
import java.util.List;
3333
import java.util.Map;
34+
import java.util.logging.Logger;
3435

3536
/**
3637
* Converts a BQ table schema to protobuf descriptor. All field names will be converted to lowercase
3738
* when constructing the protobuf descriptor. The mapping between field types and field modes are
3839
* shown in the ImmutableMaps below.
3940
*/
4041
public class BQTableSchemaToProtoDescriptor {
42+
43+
private static final Logger LOG =
44+
Logger.getLogger(BQTableSchemaToProtoDescriptor.class.getName());
45+
4146
private static Map<Mode, FieldDescriptorProto.Label> DEFAULT_BQ_TABLE_SCHEMA_MODE_MAP =
4247
ImmutableMap.of(
4348
TableFieldSchema.Mode.NULLABLE, FieldDescriptorProto.Label.LABEL_OPTIONAL,
@@ -215,26 +220,24 @@ static FieldDescriptorProto convertBQTableFieldToProtoField(
215220
case TIMESTAMP:
216221
// Can map to either int64 or string based on the BQ Field's timestamp precision
217222
// Default: microsecond (6) maps to int64 and picosecond (12) maps to string.
218-
switch ((int) BQTableField.getTimestampPrecision().getValue()) {
219-
case 12:
220-
fieldDescriptor.setType(
221-
(FieldDescriptorProto.Type) FieldDescriptorProto.Type.TYPE_STRING);
222-
break;
223-
case 6:
224-
case 0:
225-
// If the timestampPrecision value coems back as a null result from the server, a
226-
// default value
227-
// of 0L is set. Map this value as default precision as 6 (microsecond).
228-
fieldDescriptor.setType(
229-
(FieldDescriptorProto.Type) FieldDescriptorProto.Type.TYPE_INT64);
230-
break;
231-
default:
232-
// This should never happen as it's an invalid value from server
233-
throw new IllegalStateException(
234-
"BigQuery Timestamp field "
235-
+ BQTableField.getName()
236-
+ " has timestamp precision that is not 6 or 12");
223+
long timestampPrecision = BQTableField.getTimestampPrecision().getValue();
224+
if (timestampPrecision == 12L) {
225+
fieldDescriptor.setType(
226+
(FieldDescriptorProto.Type) FieldDescriptorProto.Type.TYPE_STRING);
227+
break;
228+
}
229+
// This should never happen as this is a server response issue. If this is the case,
230+
// warn the user and use INT64 as the default is microsecond precision.
231+
if (timestampPrecision != 6L || timestampPrecision != 0L) {
232+
LOG.warning(
233+
"BigQuery Timestamp field "
234+
+ BQTableField.getName()
235+
+ " has timestamp precision that is not 6 or 12. Defaulting to microsecond precision and mapping to INT64 protobuf type.");
237236
}
237+
// If the timestampPrecision value comes back as a null result from the server,
238+
// timestampPrecision has a value of 0L. Use the INT64 to map to the type used
239+
// for the default precision (microsecond).
240+
fieldDescriptor.setType((FieldDescriptorProto.Type) FieldDescriptorProto.Type.TYPE_INT64);
238241
break;
239242
default:
240243
fieldDescriptor.setType(

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1041,7 +1041,11 @@ static Instant fromEpochMicros(long micros) {
10411041
return Instant.ofEpochSecond(seconds, nanos);
10421042
}
10431043

1044-
/** Best effort to try and convert a timestamp to an ISO8601 string */
1044+
/**
1045+
* Best effort to try and convert a timestamp to an ISO8601 string. Standardize the timestamp
1046+
* output to be ISO_DATE_TIME (e.g. 2011-12-03T10:15:30+01:00) for timestamps up to nanosecond
1047+
* precision. For higher precision, the ISO8601 input is used as long as it is valid.
1048+
*/
10451049
@VisibleForTesting
10461050
static String getTimestampAsString(Object val) {
10471051
if (val instanceof String) {
@@ -1051,8 +1055,7 @@ static String getTimestampAsString(Object val) {
10511055
if (parsed != null) {
10521056
return getTimestampAsString(parsed.longValue());
10531057
}
1054-
// Validate the ISO8601 values before sending it to the server. No need to format
1055-
// if it's valid.
1058+
// Validate the ISO8601 values before sending it to the server.
10561059
validateTimestamp(value);
10571060

10581061
// If it's high precision (more than 9 digits), then return the ISO8601 string as-is
@@ -1065,8 +1068,8 @@ static String getTimestampAsString(Object val) {
10651068
Instant instant = FROM_TIMESTAMP_FORMATTER.parse(value, Instant::from);
10661069
return TO_TIMESTAMP_FORMATTER.format(instant);
10671070
} else if (val instanceof Number) {
1068-
// Micros from epoch will most likely will be represented a Long, but any non-float
1069-
// numeric value can be used
1071+
// Micros from epoch will most likely will be represented a Long, but any numeric
1072+
// value can be used
10701073
Instant instant = fromEpochMicros(((Number) val).longValue());
10711074
return TO_TIMESTAMP_FORMATTER.format(instant);
10721075
} else if (val instanceof Timestamp) {

‎google-cloud-bigquerystorage/src/test/java/com/google/cloud/bigquery/storage/v1/BQTableSchemaToProtoDescriptorTest.java‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717

1818
import static org.junit.Assert.assertEquals;
1919
import static org.junit.Assert.assertNotNull;
20-
import static org.junit.Assert.assertThrows;
2120
import static org.junit.Assert.assertTrue;
2221

2322
import com.google.cloud.bigquery.storage.test.JsonTest;
@@ -71,9 +70,9 @@ private void mapDescriptorToCount(Descriptor descriptor, HashMap<String, Integer
7170

7271
// Checks that two descriptors are the same by the check the fields inside the descriptors.
7372
// Checks that each descriptor has the same number of fields and that each field has the same
74-
// type and mode on the message. If a field is a nested message, then it recurisvly checks the
73+
// type and mode on the message. If a field is a nested message, then it recursively checks the
7574
// fields inside each nested message.
76-
private void assertDesciptorsAreEqual(Descriptor expected, Descriptor actual) {
75+
private void assertDescriptorsAreEqual(Descriptor expected, Descriptor actual) {
7776
// Check same number of fields
7877
assertEquals(actual.getFields().size(), expected.getFields().size());
7978
for (FieldDescriptor convertedField : actual.getFields()) {
@@ -90,7 +89,7 @@ private void assertDesciptorsAreEqual(Descriptor expected, Descriptor actual) {
9089
assertEquals(expectedField.isOptional(), convertedField.isOptional());
9190
// Recursively check nested messages
9291
if (convertedType == FieldDescriptor.Type.MESSAGE) {
93-
assertDesciptorsAreEqual(expectedField.getMessageType(), convertedField.getMessageType());
92+
assertDescriptorsAreEqual(expectedField.getMessageType(), convertedField.getMessageType());
9493
}
9594
}
9695
}
@@ -109,7 +108,7 @@ public void testSimpleTypes() throws Exception {
109108
TableSchema.newBuilder().addFields(0, tableFieldSchema).build();
110109
final Descriptor descriptor =
111110
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
112-
assertDesciptorsAreEqual(entry.getValue(), descriptor);
111+
assertDescriptorsAreEqual(entry.getValue(), descriptor);
113112
}
114113
}
115114

@@ -127,7 +126,7 @@ public void testTimestampType_higherTimestampPrecision()
127126
TableSchema tableSchema = TableSchema.newBuilder().addFields(0, tableFieldSchema).build();
128127
Descriptor descriptor =
129128
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
130-
assertDesciptorsAreEqual(SchemaTest.StringType.getDescriptor(), descriptor);
129+
assertDescriptorsAreEqual(SchemaTest.StringType.getDescriptor(), descriptor);
131130
}
132131

133132
@Test
@@ -197,7 +196,7 @@ public void testRange() throws Exception {
197196
.build();
198197
final Descriptor descriptor =
199198
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
200-
assertDesciptorsAreEqual(JsonTest.TestRange.getDescriptor(), descriptor);
199+
assertDescriptorsAreEqual(JsonTest.TestRange.getDescriptor(), descriptor);
201200
}
202201

203202
@Test
@@ -218,7 +217,7 @@ public void testStructSimple() throws Exception {
218217
final TableSchema tableSchema = TableSchema.newBuilder().addFields(0, tableFieldSchema).build();
219218
final Descriptor descriptor =
220219
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
221-
assertDesciptorsAreEqual(SchemaTest.MessageType.getDescriptor(), descriptor);
220+
assertDescriptorsAreEqual(SchemaTest.MessageType.getDescriptor(), descriptor);
222221
}
223222

224223
@Test
@@ -464,7 +463,7 @@ public void testStructComplex() throws Exception {
464463
.build();
465464
final Descriptor descriptor =
466465
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
467-
assertDesciptorsAreEqual(JsonTest.ComplexRoot.getDescriptor(), descriptor);
466+
assertDescriptorsAreEqual(JsonTest.ComplexRoot.getDescriptor(), descriptor);
468467
}
469468

470469
@Test
@@ -544,7 +543,7 @@ public void testCasingComplexStruct() throws Exception {
544543
.build();
545544
final Descriptor descriptor =
546545
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
547-
assertDesciptorsAreEqual(JsonTest.CasingComplex.getDescriptor(), descriptor);
546+
assertDescriptorsAreEqual(JsonTest.CasingComplex.getDescriptor(), descriptor);
548547
}
549548

550549
@Test
@@ -575,7 +574,7 @@ public void testOptions() throws Exception {
575574
.build();
576575
final Descriptor descriptor =
577576
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
578-
assertDesciptorsAreEqual(JsonTest.OptionTest.getDescriptor(), descriptor);
577+
assertDescriptorsAreEqual(JsonTest.OptionTest.getDescriptor(), descriptor);
579578
}
580579

581580
@Test
@@ -632,7 +631,7 @@ public void testDescriptorReuseDuringCreation() throws Exception {
632631
assertEquals(descriptorToCount.get("root__reuse_lvl1").intValue(), 3);
633632
assertTrue(descriptorToCount.containsKey("root__reuse_lvl1__reuse_lvl2"));
634633
assertEquals(descriptorToCount.get("root__reuse_lvl1__reuse_lvl2").intValue(), 3);
635-
assertDesciptorsAreEqual(JsonTest.ReuseRoot.getDescriptor(), descriptor);
634+
assertDescriptorsAreEqual(JsonTest.ReuseRoot.getDescriptor(), descriptor);
636635
}
637636

638637
@Test
@@ -660,7 +659,7 @@ public void testNestedFlexibleFieldName() throws Exception {
660659
TableSchema.newBuilder().addFields(0, stringField).addFields(1, nestedField).build();
661660
final Descriptor descriptor =
662661
BQTableSchemaToProtoDescriptor.convertBQTableSchemaToProtoDescriptor(tableSchema);
663-
assertDesciptorsAreEqual(SchemaTest.TestNestedFlexibleFieldName.getDescriptor(), descriptor);
662+
assertDescriptorsAreEqual(SchemaTest.TestNestedFlexibleFieldName.getDescriptor(), descriptor);
664663
}
665664

666665
@Test
@@ -691,29 +690,38 @@ public void timestampField_picosecondPrecision() throws Exception {
691690
}
692691

693692
@Test
694-
public void timestampField_picosecondPrecision_invalid() throws Exception {
693+
public void timestampField_unexpectedPrecision() throws Exception {
695694
TableFieldSchema timestampField =
696695
TableFieldSchema.newBuilder()
697696
.setType(TableFieldSchema.Type.TIMESTAMP)
698697
.setTimestampPrecision(Int64Value.newBuilder().setValue(13).build())
699698
.setMode(TableFieldSchema.Mode.NULLABLE)
700699
.build();
701-
assertThrows(
702-
IllegalStateException.class,
703-
() ->
704-
BQTableSchemaToProtoDescriptor.convertBQTableFieldToProtoField(
705-
timestampField, 0, null));
700+
DescriptorProtos.FieldDescriptorProto fieldDescriptorProto =
701+
BQTableSchemaToProtoDescriptor.convertBQTableFieldToProtoField(timestampField, 0, null);
702+
assertEquals(
703+
DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT64, fieldDescriptorProto.getType());
706704

707705
TableFieldSchema timestampField1 =
708706
TableFieldSchema.newBuilder()
709707
.setType(TableFieldSchema.Type.TIMESTAMP)
710708
.setTimestampPrecision(Int64Value.newBuilder().setValue(7).build())
711709
.setMode(TableFieldSchema.Mode.NULLABLE)
712710
.build();
713-
assertThrows(
714-
IllegalStateException.class,
715-
() ->
716-
BQTableSchemaToProtoDescriptor.convertBQTableFieldToProtoField(
717-
timestampField1, 0, null));
711+
DescriptorProtos.FieldDescriptorProto fieldDescriptorProto1 =
712+
BQTableSchemaToProtoDescriptor.convertBQTableFieldToProtoField(timestampField1, 0, null);
713+
assertEquals(
714+
DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT64, fieldDescriptorProto1.getType());
715+
716+
TableFieldSchema timestampField2 =
717+
TableFieldSchema.newBuilder()
718+
.setType(TableFieldSchema.Type.TIMESTAMP)
719+
.setTimestampPrecision(Int64Value.newBuilder().setValue(-1).build())
720+
.setMode(TableFieldSchema.Mode.NULLABLE)
721+
.build();
722+
DescriptorProtos.FieldDescriptorProto fieldDescriptorProto2 =
723+
BQTableSchemaToProtoDescriptor.convertBQTableFieldToProtoField(timestampField2, 0, null);
724+
assertEquals(
725+
DescriptorProtos.FieldDescriptorProto.Type.TYPE_INT64, fieldDescriptorProto2.getType());
718726
}
719727
}

0 commit comments

Comments
 (0)