You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "stankiewicz (via GitHub)" <gi...@apache.org> on 2025/02/27 15:26:02 UTC
[PR] fix for adding unexpected Empty Records in Nested Arrays [beam]
stankiewicz opened a new pull request, #34102:
URL: https://github.com/apache/beam/pull/34102
fixes #33842
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #34102:
URL: https://github.com/apache/beam/pull/34102#discussion_r1979496089
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java:
##########
@@ -1671,6 +1673,85 @@ public void testIgnoreUnknownRepeatedNestedField() throws Exception {
assertEquals("foobar_doubly_nested", unknownDoublyNestedStruct.get("unknown_doubly_nested"));
}
+ @Test
+ public void testIgnoreUnknownRepeatedNestedFieldWithNoUnknowns() throws Exception {
+
+ List<TableFieldSchema> fields = new ArrayList<>();
+ fields.add(new TableFieldSchema().setName("foo").setType("STRING"));
+ fields.add(
+ new TableFieldSchema()
+ .setName("repeated1")
+ .setMode("REPEATED")
+ .setType("RECORD")
+ .setFields(
+ ImmutableList.of(
+ new TableFieldSchema().setName("key1").setType("STRING").setMode("REQUIRED"),
+ new TableFieldSchema().setName("key2").setType("STRING"))));
+ TableSchema schema = new TableSchema().setFields(fields);
+ TableRow tableRow =
+ new TableRow()
+ .set("foo", "bar")
+ .set(
+ "repeated1",
+ ImmutableList.of(
+ new TableCell().set("key1", "valueA").set("key2", "valueC"),
+ new TableCell().set("key1", "valueB").set("key2", "valueD")));
+
+ Descriptor descriptor =
+ TableRowToStorageApiProto.getDescriptorFromTableSchema(schema, true, false);
+ TableRowToStorageApiProto.SchemaInformation schemaInformation =
+ TableRowToStorageApiProto.SchemaInformation.fromTableSchema(schema);
+ TableRow unknown = new TableRow();
+ DynamicMessage msg =
+ TableRowToStorageApiProto.messageFromTableRow(
+ schemaInformation, descriptor, tableRow, true, false, unknown, null, -1);
+ assertEquals(2, msg.getAllFields().size());
+ assertTrue(unknown.isEmpty());
+ }
+
+ @Test
+ public void testIgnoreUnknownRepeatedNestedFieldWithUknownInRepeatedField() throws Exception {
+
+ List<TableFieldSchema> fields = new ArrayList<>();
+ fields.add(new TableFieldSchema().setName("foo").setType("STRING"));
+ fields.add(
+ new TableFieldSchema()
+ .setName("repeated1")
+ .setMode("REPEATED")
+ .setType("RECORD")
+ .setFields(
+ ImmutableList.of(
+ new TableFieldSchema().setName("key1").setType("STRING").setMode("REQUIRED"),
+ new TableFieldSchema().setName("key2").setType("STRING"))));
+ TableSchema schema = new TableSchema().setFields(fields);
+ TableRow tableRow =
+ new TableRow()
+ .set("foo", "bar")
+ .set(
+ "repeated1",
+ ImmutableList.of(
+ new TableCell().set("key1", "valueA").set("key2", "valueC"),
+ new TableCell()
+ .set("key1", "valueB")
+ .set("key2", "valueD")
+ .set("unknown", "valueE")));
+
+ Descriptor descriptor =
+ TableRowToStorageApiProto.getDescriptorFromTableSchema(schema, true, false);
+ TableRowToStorageApiProto.SchemaInformation schemaInformation =
+ TableRowToStorageApiProto.SchemaInformation.fromTableSchema(schema);
+ TableRow unknown = new TableRow();
+ DynamicMessage msg =
+ TableRowToStorageApiProto.messageFromTableRow(
+ schemaInformation, descriptor, tableRow, true, false, unknown, null, -1);
+ assertEquals(2, msg.getAllFields().size());
+ assertFalse(unknown.isEmpty());
+ assertEquals(2, ((List<?>) unknown.get("repeated1")).size());
+ assertNotNull(((List<?>) unknown.get("repeated1")).get(0));
+ assertNotNull(((List<?>) unknown.get("repeated1")).get(1));
+ assertEquals("valueE", ((TableRow) ((List<?>) unknown.get("repeated1")).get(1)).get("unknown"));
Review Comment:
so if i'm following correctly, index 1 of `repeated` list is `{unknown: valueE}`, and index 0 is an empty map? can we add a check that it's empty?
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java:
##########
@@ -1671,6 +1673,85 @@ public void testIgnoreUnknownRepeatedNestedField() throws Exception {
assertEquals("foobar_doubly_nested", unknownDoublyNestedStruct.get("unknown_doubly_nested"));
}
+ @Test
+ public void testIgnoreUnknownRepeatedNestedFieldWithNoUnknowns() throws Exception {
+
+ List<TableFieldSchema> fields = new ArrayList<>();
+ fields.add(new TableFieldSchema().setName("foo").setType("STRING"));
+ fields.add(
+ new TableFieldSchema()
+ .setName("repeated1")
+ .setMode("REPEATED")
+ .setType("RECORD")
+ .setFields(
+ ImmutableList.of(
+ new TableFieldSchema().setName("key1").setType("STRING").setMode("REQUIRED"),
+ new TableFieldSchema().setName("key2").setType("STRING"))));
+ TableSchema schema = new TableSchema().setFields(fields);
+ TableRow tableRow =
+ new TableRow()
+ .set("foo", "bar")
+ .set(
+ "repeated1",
+ ImmutableList.of(
+ new TableCell().set("key1", "valueA").set("key2", "valueC"),
+ new TableCell().set("key1", "valueB").set("key2", "valueD")));
+
+ Descriptor descriptor =
+ TableRowToStorageApiProto.getDescriptorFromTableSchema(schema, true, false);
+ TableRowToStorageApiProto.SchemaInformation schemaInformation =
+ TableRowToStorageApiProto.SchemaInformation.fromTableSchema(schema);
+ TableRow unknown = new TableRow();
+ DynamicMessage msg =
+ TableRowToStorageApiProto.messageFromTableRow(
+ schemaInformation, descriptor, tableRow, true, false, unknown, null, -1);
+ assertEquals(2, msg.getAllFields().size());
+ assertTrue(unknown.isEmpty());
+ }
+
+ @Test
+ public void testIgnoreUnknownRepeatedNestedFieldWithUknownInRepeatedField() throws Exception {
Review Comment:
nit
```suggestion
public void testIgnoreUnknownRepeatedNestedFieldWithUnknownInRepeatedField() throws Exception {
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java:
##########
@@ -536,11 +536,20 @@ public static DynamicMessage messageFromMap(
// For STRUCT fields, we add a placeholder to unknownFields using the getNestedUnknown
// supplier (in case we encounter unknown nested fields). If the placeholder comes out
// to be empty, we should clean it up
+
if (fieldSchemaInformation.getType().equals(TableFieldSchema.Type.STRUCT)
- && unknownFields != null
- && unknownFields.get(key) instanceof Map
- && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) {
- unknownFields.remove(key);
+ && unknownFields != null) {
+ if (unknownFields.get(key) instanceof Map
+ && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) {
+ unknownFields.remove(key);
+ } else if (unknownFields.get(key) instanceof List) { // repeated
+
+ if (((List<?>) unknownFields.get(key)).isEmpty()
+ || ((List<?>) unknownFields.get(key))
+ .stream().allMatch(row -> row == null || ((Map<?, ?>) row).isEmpty())) {
Review Comment:
combine these two if-clauses together?
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz commented on PR #34102:
URL: https://github.com/apache/beam/pull/34102#issuecomment-2693724746
do not merge yet, I think I found another bug.
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #34102:
URL: https://github.com/apache/beam/pull/34102
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #34102:
URL: https://github.com/apache/beam/pull/34102#discussion_r1977311213
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java:
##########
@@ -1671,6 +1672,44 @@ public void testIgnoreUnknownRepeatedNestedField() throws Exception {
assertEquals("foobar_doubly_nested", unknownDoublyNestedStruct.get("unknown_doubly_nested"));
}
+ @Test
+ public void testIgnoreUnknownRepeatedNestedField2() throws Exception {
+
+ List<TableFieldSchema> fields = new ArrayList<>();
+ fields.add(new TableFieldSchema().setName("foo").setType("STRING"));
+ fields.add(
+ new TableFieldSchema()
+ .setName("repeated1")
+ .setMode("REPEATED")
+ .setType("RECORD")
+ .setFields(
+ ImmutableList.of(
+ new TableFieldSchema().setName("key1").setType("STRING").setMode("REQUIRED"),
+ new TableFieldSchema().setName("key2").setType("STRING"))));
+ TableSchema schema = new TableSchema().setFields(fields);
+ TableRow tableRow =
+ new TableRow()
+ .set("foo", "bar")
+ .set(
+ "repeated1",
+ ImmutableList.of(
+ new TableCell().set("key1", "valueA").set("key2", "valueC"),
+ new TableCell().set("key1", "valueB").set("key2", "valueD")));
+
+ Descriptor descriptor =
+ TableRowToStorageApiProto.getDescriptorFromTableSchema(schema, true, false);
+ TableRowToStorageApiProto.SchemaInformation schemaInformation =
+ TableRowToStorageApiProto.SchemaInformation.fromTableSchema(schema);
+ TableRow unknown = new TableRow();
+ DynamicMessage msg =
+ TableRowToStorageApiProto.messageFromTableRow(
+ schemaInformation, descriptor, tableRow, true, false, unknown, null, -1);
+ assertEquals(2, msg.getAllFields().size());
+
+ System.out.println(unknown);
Review Comment:
p.s. cleanup
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34102:
URL: https://github.com/apache/beam/pull/34102#issuecomment-2688654523
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @robertwb for label java.
R: @damondouglas for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #34102:
URL: https://github.com/apache/beam/pull/34102#discussion_r1977302390
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProtoTest.java:
##########
@@ -1671,6 +1672,44 @@ public void testIgnoreUnknownRepeatedNestedField() throws Exception {
assertEquals("foobar_doubly_nested", unknownDoublyNestedStruct.get("unknown_doubly_nested"));
}
+ @Test
+ public void testIgnoreUnknownRepeatedNestedField2() throws Exception {
Review Comment:
nit: more descriptive test name
```suggestion
public void testIgnoreUnknownRepeatedNestedFieldWithNoUnknowns() throws Exception {
```
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz commented on PR #34102:
URL: https://github.com/apache/beam/pull/34102#issuecomment-2690464751
@ahmedabu98 can you take a look at this? 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz commented on PR #34102:
URL: https://github.com/apache/beam/pull/34102#issuecomment-2694187730
issue I see additional problem here: [StorageApiWriteUnshardedRecords](https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/StorageApiWriteUnshardedRecords.java#L587)
If we have a row with known fields (name:string, surname:string) and one unknown (foo:string) , this line will concat and not cause schema issues.
but if unknown field is part of repeated structure,
e.g. (name:string, phone:(repeated struct type:string, num:string) ) and we have unknown in repeated struct (phone: [ null, {favourite:yes} ]) concat doesn't make any sense - should be mix of merge & concat at each level.
For current bug, it is resolved - it won't add empty records so we can merge it.
Added issue for concatenating unknown nested fields which may require bigger changes #34145
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #34102:
URL: https://github.com/apache/beam/pull/34102#discussion_r1979648985
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java:
##########
@@ -539,16 +539,14 @@ public static DynamicMessage messageFromMap(
if (fieldSchemaInformation.getType().equals(TableFieldSchema.Type.STRUCT)
&& unknownFields != null) {
- if (unknownFields.get(key) instanceof Map
- && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) {
+ if ((unknownFields.get(key) instanceof Map
+ && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) // single struct, empty
+ || (unknownFields.get(key)
+ instanceof List // repeated struct, empty list or list with empty structs
+ && (((List<?>) unknownFields.get(key)).isEmpty()
+ || ((List<?>) unknownFields.get(key))
+ .stream().allMatch(row -> row == null || ((Map<?, ?>) row).isEmpty())))) {
Review Comment:
I guess we can also merge with the outer if-statement? this is gonna be a monster of an if-statement.
Maybe there's a way to clean it up, but it's not PR-blocking so will leave it up to you.
--
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
Re: [PR] fix for adding unexpected Empty Records in Nested Arrays in BigQueryIO [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz commented on code in PR #34102:
URL: https://github.com/apache/beam/pull/34102#discussion_r1979934652
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java:
##########
@@ -539,16 +539,14 @@ public static DynamicMessage messageFromMap(
if (fieldSchemaInformation.getType().equals(TableFieldSchema.Type.STRUCT)
&& unknownFields != null) {
- if (unknownFields.get(key) instanceof Map
- && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) {
+ if ((unknownFields.get(key) instanceof Map
+ && ((Map<?, ?>) unknownFields.get(key)).isEmpty()) // single struct, empty
+ || (unknownFields.get(key)
+ instanceof List // repeated struct, empty list or list with empty structs
+ && (((List<?>) unknownFields.get(key)).isEmpty()
+ || ((List<?>) unknownFields.get(key))
+ .stream().allMatch(row -> row == null || ((Map<?, ?>) row).isEmpty())))) {
Review Comment:
Done and green
--
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