Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-13945] (FIX) Update Java BQ connector to support new JSON type #17492

Merged
merged 5 commits into from
Jun 2, 2022

Conversation

ahmedabu98
Copy link
Contributor

@ahmedabu98 ahmedabu98 commented Apr 28, 2022

Follow up to #17209.
More thorough way to look for JSON column type in schema and throw an error when the user is writing to BigQuery with the FILE_LOADS write method. The error is thrown because JSON insertion is currently not supported with batch loads.

@asf-ci
Copy link

asf-ci commented Apr 28, 2022

Can one of the admins verify this patch?

1 similar comment
@asf-ci
Copy link

asf-ci commented Apr 28, 2022

Can one of the admins verify this patch?

@ahmedabu98
Copy link
Contributor Author

Run PostCommit_Java_DataflowV2

@ahmedabu98
Copy link
Contributor Author

retest this please

@ahmedabu98
Copy link
Contributor Author

R: @pabloem
R: @johnjcasey

Fixing the way we look for JSON column types in the schema.

+ "json-data#ingest_json_data");

if (field.getString("type").equals("STRUCT")) {
validateNoJsonTypeInSchema(field);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks basically good. Are we at all concerned with recursion depth here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BigQuery docs show that the limit is 15 nested structs. If the user sticks to the BigQuery guidelines it shouldn't be a problem.

Ofc this doesn't stop the user from making a mistake. Do you think it's necessary for us to handle that here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, if the bq limit is 15. LGTM

@pabloem
Copy link
Member

pabloem commented May 3, 2022

Run Java PreCommit

@pabloem
Copy link
Member

pabloem commented May 3, 2022

@ahmedabu98 I think this change breaks this test: https://ci-beam.apache.org/job/beam_PreCommit_Java_Phrase/4995/testReport/junit/org.apache.beam.sdk.io.gcp.bigquery/BigQueryServicesImplTest/testInsertWithinRowCountLimits/

You can run it locally with ./gradlew :sdks:java:io:google-cloud-platform:test --tests *testInsertWithinRowCountLimits*

@ahmedabu98
Copy link
Contributor Author

@pabloem the BigQueryServicesImplTest.testInsertWithinRowCountLimits test might be flaky. It's passing now with another PreCommit test run.

@aaltay
Copy link
Member

aaltay commented May 12, 2022

What is the next step on this PR?

@ahmedabu98
Copy link
Contributor Author

@pabloem precommit test fails are unrelated

@ahmedabu98
Copy link
Contributor Author

retest this please

@ahmedabu98
Copy link
Contributor Author

Run Kotlin_Examples PreCommit

@pabloem
Copy link
Member

pabloem commented Jun 1, 2022

Run Java PreCommit

2 similar comments
@pabloem
Copy link
Member

pabloem commented Jun 1, 2022

Run Java PreCommit

@pabloem
Copy link
Member

pabloem commented Jun 2, 2022

Run Java PreCommit

@pabloem
Copy link
Member

pabloem commented Jun 2, 2022

Run Java PostCommit

@pabloem
Copy link
Member

pabloem commented Jun 2, 2022

Run Java PreCommit

@pabloem
Copy link
Member

pabloem commented Jun 2, 2022

LGTM.

@pabloem pabloem merged commit 12be69d into apache:master Jun 2, 2022
@lukecwik
Copy link
Member

lukecwik commented Jun 3, 2022

I believe this is the cause for the permared Java PreCommit which is failing due to:

09:55:24 * What went wrong:
09:55:24 Execution failed for task ':sdks:java:io:google-cloud-platform:analyzeClassesDependencies'.
09:55:24 > Dependency analysis found issues.
09:55:24   usedUndeclaredArtifacts
09:55:24    - org.json:json:20200518@jar

pabloem added a commit to pabloem/beam that referenced this pull request Jun 3, 2022
…e Java BQ connector to support new JSON type "

This reverts commit 12be69d.
@pabloem
Copy link
Member

pabloem commented Jun 3, 2022

sorry about that. I've opened a rollback and a fix forward:

#18038
#18060

pabloem added a commit that referenced this pull request Jun 3, 2022
…BEAM-13945] (FIX) Update Java…

Revert "Merge pull request #17492 from [BEAM-13945] (FIX) Update Java…
pabloem added a commit to pabloem/beam that referenced this pull request Jun 3, 2022
…X) Update Java BQ connector to support new JSON type ""

This reverts commit 688d183.
pabloem added a commit that referenced this pull request Jun 3, 2022
… for BQIO

* Revert "Revert "Merge pull request #17492 from [BEAM-13945] (FIX) Update Java BQ connector to support new JSON type ""

This reverts commit 688d183.

* cherry-pick @johnjcasey fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants