TheNeuralBit commented on a change in pull request #12711:
URL: https://github.com/apache/beam/pull/12711#discussion_r487193734
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
##########
File path:
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java
##########
@@ -257,10 +263,9 @@ private static FieldType fromTableFieldSchemaType(
case "DATE":
return FieldType.logicalType(SqlTypes.DATE);
case "DATETIME":
- // TODO[BEAM-10240]: map to the new logical type when ZetaSQL DATETIME
type is supported
- return FieldType.logicalType(
- new PassThroughLogicalType<Instant>(
- "SqlTimestampWithLocalTzType", FieldType.STRING, "",
FieldType.DATETIME) {});
+ return FieldType.logicalType(SqlTypes.DATETIME);
Review comment:
It's possible for users of BigQueryIO to hit this code path through
`BigQueryIO.readTableRowsWithSchema()` right? This will be a breaking change
since the Java type for a DATETIME field changes from Instant to LocalDateTime.
I think that's ok since it's experimental, but we should add a note to
CHANGES.md
##########
File path:
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java
##########
@@ -452,6 +457,27 @@ public void testMicroPrecisionTimeType() {
equalTo(t));
}
+ @Test
+ public void testMicroPrecisionDateTimeType() {
+ LocalDateTime dt = LocalDateTime.parse("2020-06-04T12:34:56.789876");
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.logicalType(SqlTypes.DATETIME), new Utf8(dt.toString()),
REJECT_OPTIONS),
+ equalTo(dt));
+ }
+
+ @Test
+ public void testNumericType() {
+ // BigQuery NUMERIC type has precision 38 and scale 9
+ BigDecimal n = new BigDecimal("123456789.987654321").setScale(9);
+ assertThat(
+ BigQueryUtils.convertAvroFormat(
+ FieldType.DECIMAL,
+ new Conversions.DecimalConversion().toBytes(n, null,
LogicalTypes.decimal(38, 9)),
+ REJECT_OPTIONS),
+ equalTo(n));
+ }
+
Review comment:
Could you make sure that `BigQueryUtils.fromTableSchema` is tested with
NUMERIC and DATETIME? It looks like only TIMESTAMP may be tested now (as part
of `testTableFromSchema_flat`):
https://github.com/apache/beam/blob/738a9100ec5ce09870729b64e1ef483f692134c2/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java#L91-L106
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
