You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "jonathan-lemos (via GitHub)" <gi...@apache.org> on 2023/05/24 18:08:34 UTC
[GitHub] [beam] jonathan-lemos opened a new pull request, #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
jonathan-lemos opened a new pull request, #26873:
URL: https://github.com/apache/beam/pull/26873
Fixes #26870
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1564976854
Run Spotless 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jonathan-lemos commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "jonathan-lemos (via GitHub)" <gi...@apache.org>.
jonathan-lemos commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1204723955
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,7 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(v -> v instanceof Map ? ((Map<String, Object>) v).get("v") : v)
Review Comment:
I have covered this case.
Interestingly, it doesn't look like BigQuery supports a Map type (https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#data_type_sizes), so I don't know how to test this path.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,7 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(v -> v instanceof Map ? ((Map<String, Object>) v).get("v") : v)
Review Comment:
I have covered this case in the latest commit.
Interestingly, it doesn't look like BigQuery supports a Map type (https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#data_type_sizes), so I don't know how to test this path.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1561720233
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ahmedabu98 commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1204630625
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,7 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(v -> v instanceof Map ? ((Map<String, Object>) v).get("v") : v)
Review Comment:
Let's also check that the fieldType.getCollectionElementType() isn't a Map.
ie. we should also skip the `get("v")"` operation if the user is converting a TableRow value to a List<Maps<X, Y>>.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jonathan-lemos commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "jonathan-lemos (via GitHub)" <gi...@apache.org>.
jonathan-lemos commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1207255593
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,15 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v ->
+ (v instanceof Map
+ && !fieldType
+ .getCollectionElementType()
Review Comment:
`fieldType.getCollectionElementType()` should only return `null` if the fieldType is not a collection, but the `if (jsonBQValue instanceof List)` case should cover that scenario. However, there's nothing stopping someone from passing in a non-collection FieldType with a List value, so my latest changes show a more helpful error message in that case.
I've extracted the `!fieldType.getCollectionElementType().getTypeName().equals(TypeName.MAP)` to a variable outside of the lambda.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1207201569
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,15 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v ->
+ (v instanceof Map
+ && !fieldType
+ .getCollectionElementType()
Review Comment:
and `!fieldType.getCollectionElementType().getTypeName().equals(TypeName.MAP)` does not depend on "v" and can be extract outside the lambda (for visibility)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jonathan-lemos commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "jonathan-lemos (via GitHub)" <gi...@apache.org>.
jonathan-lemos commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1205863520
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,16 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v -> {
+ if (v instanceof Map) {
+ Map<String, Object> m = (Map<String, Object>) v;
+ if (m.size() == 1 && m.containsKey("v")) {
+ return m.get("v");
+ }
+ }
+ return v;
+ })
Review Comment:
That makes more sense. I've applied the change and formatted it.
Trying to make a test case for this scenario, it seems that Beam's BigQuery IO will flatten any list of structs e.g. querying
```
[
{make: honda, metadata: [{color: red, model: civic}, {color: blue, model: accord}]}
{make: toyota, metadata: [{color: green, model: corolla}]}
]
```
will result in the following:
```
[
{make: honda, metadata_color: red, metadata_model: civic}
{make: honda, metadata_color: blue, metadata_model: accord}
{make: toyota, metadata_color: green, metadata_model: corolla}
]
```
The underlying BigQuery API returns the two rows above, but Beam's [`readTableRows()`](https://beam.apache.org/releases/javadoc/current/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.html#readTableRows--) will flatten the structure and return the 3 below.
Given that behavior, it doesn't seem like it's possible for a value of `List<Struct>` to make it to this function, so I haven't added a test case for that scenario.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ahmedabu98 commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1204765040
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,7 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(v -> v instanceof Map ? ((Map<String, Object>) v).get("v") : v)
Review Comment:
The Map refers to the Beam Schema type, I think the equivalent for BigQuery schema might be a struct
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,16 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v -> {
+ if (v instanceof Map) {
+ Map<String, Object> m = (Map<String, Object>) v;
+ if (m.size() == 1 && m.containsKey("v")) {
+ return m.get("v");
+ }
+ }
+ return v;
+ })
Review Comment:
I was thinking something like this, just to check if the user is intentionally converting a list of maps.
```suggestion
.map(v -> (v instanceof Map && !fieldType.getCollectionElementType().getTypeName().equals(TypeName.MAP)) ? ((Map<String, Object>) v).get("v") : v)
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ahmedabu98 commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1204758298
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,16 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v -> {
+ if (v instanceof Map) {
+ Map<String, Object> m = (Map<String, Object>) v;
+ if (m.size() == 1 && m.containsKey("v")) {
+ return m.get("v");
+ }
+ }
+ return v;
+ })
Review Comment:
I was thinking something like this, just to check if the user is intentionally using a list of maps.
```suggestion
.map(v -> (v instanceof Map && !fieldType.getCollectionElementType().getTypeName().equals(TypeName.MAP)) ? ((Map<String, Object>) v).get("v") : v)
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,7 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(v -> v instanceof Map ? ((Map<String, Object>) v).get("v") : v)
Review Comment:
The Map refers to the Beam Schema type, I think the equivalent for BigQuery schema might be a struct
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1207201569
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,15 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v ->
+ (v instanceof Map
+ && !fieldType
+ .getCollectionElementType()
Review Comment:
and `!fieldType.getCollectionElementType().getTypeName().equals(TypeName.MAP)` does not depend on and can be extract outside the lambda (for visibility)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ahmedabu98 commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1207196178
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,16 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v -> {
+ if (v instanceof Map) {
+ Map<String, Object> m = (Map<String, Object>) v;
+ if (m.size() == 1 && m.containsKey("v")) {
+ return m.get("v");
+ }
+ }
+ return v;
+ })
Review Comment:
Users can set this option to get results without flattening:
https://github.com/apache/beam/blob/6865eefac5633198c71fd1ec75b514e8a390d002/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java#L823
A test for List<Map<>> should be added at some point, but let's not block this PR for that
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1564977067
Run Kotlin_Examples 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] jonathan-lemos commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "jonathan-lemos (via GitHub)" <gi...@apache.org>.
jonathan-lemos commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1561718906
R: @ahmedabu98
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1564976558
Run RAT 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn merged pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #26873:
URL: https://github.com/apache/beam/pull/26873
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #26873:
URL: https://github.com/apache/beam/pull/26873#issuecomment-1564899430
retest this please
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #26873: Fix ClassCastException in toBeamValue with List<{Primitive}> types
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #26873:
URL: https://github.com/apache/beam/pull/26873#discussion_r1207199251
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -694,7 +694,15 @@ public static Row toBeamRow(Schema rowSchema, TableSchema bqSchema, TableRow jso
if (jsonBQValue instanceof List) {
return ((List<Object>) jsonBQValue)
.stream()
- .map(v -> ((Map<String, Object>) v).get("v"))
+ .map(
+ v ->
+ (v instanceof Map
+ && !fieldType
+ .getCollectionElementType()
Review Comment:
are their NPE concern? fieldType.getCollectionElementType is nullable
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org