You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/08/27 23:48:13 UTC
[GitHub] [beam] robinyqiu opened a new pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
robinyqiu opened a new pull request #12711:
URL: https://github.com/apache/beam/pull/12711
Add support to 2 more new types: DATETIME and NUMERIC, similar to https://github.com/apache/beam/pull/12261
Also add the missing mapping for NUMERIC in SchemaUtils in Data Catalog.
r: @chamikaramj, @TheNeuralBit
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace
--- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12711:
URL: https://github.com/apache/beam/pull/12711#issuecomment-685022550
Run Java PreCommit
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [beam] robinyqiu merged pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
robinyqiu merged pull request #12711:
URL: https://github.com/apache/beam/pull/12711
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12711:
URL: https://github.com/apache/beam/pull/12711#discussion_r516327664
##########
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:
Done. Thanks for catching this.
##########
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:
Done. Added test for new types: DATE, TIME, and DATETIME
##########
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:
Done. Added test for new types: DATE, TIME, and DATETIME.
Tests for NUMERICS is already added in https://github.com/apache/beam/pull/12730
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12711:
URL: https://github.com/apache/beam/pull/12711#issuecomment-685881983
Run Java PreCommit
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12711:
URL: https://github.com/apache/beam/pull/12711#issuecomment-721377621
@chamikaramj @TheNeuralBit Rebased the PR and addressed the comments. PTAL, thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #12711: Support read/write ZetaSQL DATETIME/NUMERIC types from/to BigQuery
Posted by GitBox <gi...@apache.org>.
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
----------------------------------------------------------------
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:
users@infra.apache.org