[BEAM-7755] adding support for nested rows and arrays in BigQuery to Beam row conversion by snallapa · Pull Request #9089 · apache/beam

snallapa · 2019-07-17T14:05:01Z

Example implementation of repeated record conversion from BigQuery to beam row through avro. Sorry this is my first contribution so I am a bit new to this. I wanted to put this up as this worked on a test table with a REPEATED RECORD that I tried on and maybe just put it on the radar?
R: @lukecwik
R: @chamikaramj

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Gearpump	Samza
Go	---	---	---	---
Java
Python	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

amaliujia · 2019-07-24T17:09:23Z

Could you add a test, which will better illustrate what this change is about?

I think you are trying to handle the case of ROW<ROW<ROW<..>>>

snallapa · 2019-07-24T18:30:56Z

@amaliujia hey I added a test. I will clean it up later but I wanted to have it done to show you the problem. It is the case you are talking about and that test fails without the change I made. I also want to point out that all the tests in that file there use a certain toBeamRow method and the one I found this issue on did not have tests for. Is the toBeamRow method that goes through Avro not recommended?

kennknowles

Thanks for working on this. It is a major limitation! I left some comments. Basically to make it work more generally. It will not really be harder than what you have done, just a tiny refactor.

kennknowles · 2019-07-25T19:18:06Z

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java

-      ret.add(
-          convertAvroPrimitiveTypes(
-              beamField.getType().getCollectionElementType().getTypeName(), v));
+      FieldType arrayElementType = beamField.getType().getCollectionElementType();


This, and a little more below, can move out of the loop. Actually you can move it out of the method entirely, I think. You don't need to pass the whole Field but it should only depend on the type, right?

+1

values actually share the same type because it is an ARRAY. At least collection element type can be removed out of loop

kennknowles · 2019-07-25T19:20:59Z

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java

+        GenericData.Record record = (GenericData.Record) v;
+        ret.add(toBeamRow(record, arrayElementType.getRowSchema(), options));
+      } else {
+        ret.add(convertAvroPrimitiveTypes(elementTypeName, v));


I think this if / else piece should just be a recursive call into convertAvroFormat. You will need to update it to only need the FieldType and not the whole Field. This should be easy, I think. I just read the code and it does not seem to actually need the whole Field.

big +1.

Using convertAvroFormat will make sure all other types (not limited to ROW or primitive) can be handled. It that case this is changed to handle ARRAY<T>, not limited to ARRAY<ROW> or ARRAY<primitive type>

The only spot that needed the entire Field was using its name, so unfortunately if we encounter an error we can't inform the user what Field name caused that error (but we can at least tell them the type)

amaliujia

Thanks for the update since last time.

amaliujia · 2019-07-25T20:09:21Z

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java

-      ret.add(
-          convertAvroPrimitiveTypes(
-              beamField.getType().getCollectionElementType().getTypeName(), v));
+      FieldType arrayElementType = beamField.getType().getCollectionElementType();


+1

values actually share the same type because it is an ARRAY. At least collection element type can be removed out of loop

amaliujia · 2019-07-25T20:12:32Z

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java

+        GenericData.Record record = (GenericData.Record) v;
+        ret.add(toBeamRow(record, arrayElementType.getRowSchema(), options));
+      } else {
+        ret.add(convertAvroPrimitiveTypes(elementTypeName, v));


big +1.

Using convertAvroFormat will make sure all other types (not limited to ROW or primitive) can be handled. It that case this is changed to handle ARRAY<T>, not limited to ARRAY<ROW> or ARRAY<primitive type>

...ogle-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtilsTest.java

snallapa · 2019-07-26T13:40:34Z

Thanks to both of you for the feedback! I updated my branch with some changes and I added one more test as well.

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java

kennknowles

Very nice improvement! I will wait for the test results and then merge.

snallapa · 2019-07-26T20:16:02Z

thanks!! @kennknowles

Sahith Nallapareddy added 2 commits July 17, 2019 09:13

adding repeated row implementation

307c6b3

building

17b323a

adding test

ee3462b

Sahith Nallapareddy added 2 commits July 24, 2019 14:36

spotless

b90d1e3

fixed naming

2caa501

kennknowles requested changes Jul 25, 2019

View reviewed changes

amaliujia reviewed Jul 25, 2019

View reviewed changes

making code more general and adding ARRAY<ARRAY<ROW>> test

c18f95e

kennknowles reviewed Jul 26, 2019

View reviewed changes

...o/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java Outdated Show resolved Hide resolved

adding error message

c2d387a

kennknowles approved these changes Jul 26, 2019

View reviewed changes

kennknowles changed the title ~~[BEAM-7755] adding repeated row implementation that worked on a simple case~~ [BEAM-7755] adding support for nested rows and arrays in BigQuery to Beam row conversion Jul 26, 2019

kennknowles self-requested a review July 26, 2019 19:03

kennknowles merged commit 389f649 into apache:master Jul 26, 2019

jubebo mentioned this pull request Jun 19, 2023

[Feature Request]: Add support for BigQuery structs in BigQueryIO with BEAM_ROW output type #27166

Open

15 tasks

Conversation

snallapa commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

amaliujia commented Jul 24, 2019

Uh oh!

snallapa commented Jul 24, 2019

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

kennknowles Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

amaliujia Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennknowles Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

amaliujia Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

snallapa Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

amaliujia left a comment

Choose a reason for hiding this comment

Uh oh!

amaliujia Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snallapa commented Jul 26, 2019

Uh oh!

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

snallapa commented Jul 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snallapa commented Jul 17, 2019 •

edited

Loading

amaliujia Jul 25, 2019 •

edited

Loading

amaliujia Jul 25, 2019 •

edited

Loading