Skip to content

Conversation

@ElliottBrossard
Copy link

The NUMERIC data type is represented in Avro using the BYTES type with the DECIMAL logical type.

I verified this change using BigQueryTornadoes.java, which I modified (locally) to write NUMERIC values to BigQuery, then read them back using Dataflow and write them to a new BigQuery table. The test failed as expected without this change, but now passes, and I can see the correct content in the table.

Context: #5755 (comment)

…m Avro.

The NUMERIC data type is represented in Avro using the BYTES type with the DECIMAL logical type.

I verified this change using BigQueryTornadoes.java, which I modified (locally) to write NUMERIC values to BigQuery, then read them back using Dataflow and write them to a new BigQuery table. The test failed as expected without this change, but now passes, and I can see the correct content in the table.

Context: #5755 (comment)
@ElliottBrossard
Copy link
Author

@kennknowles I'd appreciate it if you could take a look at this PR as well, thanks!

@ElliottBrossard ElliottBrossard changed the title [BEAM-4706] Fix the expected encoding of BigQuery's NUMERIC type when reading from Avro [BEAM-4417] Fix the expected encoding of BigQuery's NUMERIC type when reading from Avro Jul 13, 2018
@aaltay
Copy link
Member

aaltay commented Jul 13, 2018

I will review this today. Thank you @ElliottBrossard

@aaltay
Copy link
Member

aaltay commented Jul 13, 2018

Run Java PostCommit

@aaltay
Copy link
Member

aaltay commented Jul 14, 2018

The change LGTM, however it is breaking tests. :beam-sdks-java-extensions-sql:integrationTest is the failing test.

cc: @akedin will take a look it error.

@akedin
Copy link
Contributor

akedin commented Jul 14, 2018

Looking at it, can reproduce, but not sure how these changes could have affected the failing test. Probably it was already broken, checking on master

@akedin
Copy link
Contributor

akedin commented Jul 14, 2018

Ah, it fails on master and I think it's a known issue. @apilloud , @amaliujia can you take a look?

@amaliujia
Copy link
Contributor

@akedin

Yes it is a known issue and fixed by #5930

@amaliujia
Copy link
Contributor

Run Java PostCommit

@akedin
Copy link
Contributor

akedin commented Jul 16, 2018

LGTM

@ElliottBrossard
Copy link
Author

@aaltay, any comments? Thanks!

@aaltay aaltay merged commit ae2beba into apache:master Jul 17, 2018
@aaltay
Copy link
Member

aaltay commented Jul 17, 2018

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants