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 ---------------------------------------------------------------- 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: us...@infra.apache.org