You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2025/07/15 17:30:59 UTC
[PR] Fix json schema test [beam]
ahmedabu98 opened a new pull request, #35597:
URL: https://github.com/apache/beam/pull/35597
Test was sometimes failing because it validated using strict equality, but json schema with multiple nullable fields can result in random Beam schema field ordering:
https://github.com/apache/beam/blob/1fd8ce382d71ea5cb37281cf0b9f439ef3b87734/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/JsonUtils.java#L231-L233
Fixing test by checking for equivalence, which permits fields being out of order
--
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 json schema test [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #35597:
URL: https://github.com/apache/beam/pull/35597
--
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 json schema test [beam]
Posted by "acrites (via GitHub)" <gi...@apache.org>.
acrites commented on code in PR #35597:
URL: https://github.com/apache/beam/pull/35597#discussion_r2208155037
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JsonSchemaConversionTest.java:
##########
@@ -285,24 +287,31 @@ public void testArrayWithNestedRefsBeamSchema() throws IOException {
String stringJsonSchema = new String(ByteStreams.toByteArray(inputStream), "UTF-8");
Schema parsedSchema = JsonUtils.beamSchemaFromJsonSchema(stringJsonSchema);
- assertThat(parsedSchema.getFieldNames(), containsInAnyOrder("vegetables"));
- assertThat(
- parsedSchema.getFields().stream().map(Schema.Field::getType).collect(Collectors.toList()),
- containsInAnyOrder(
- Schema.FieldType.array(
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.of("veggieName", Schema.FieldType.STRING),
- Schema.Field.of("veggieLike", Schema.FieldType.BOOLEAN),
- Schema.Field.nullable(
- "origin",
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.nullable("country", Schema.FieldType.STRING),
- Schema.Field.nullable("town", Schema.FieldType.STRING),
- Schema.Field.nullable(
- "region", Schema.FieldType.STRING)))))))
- .withNullable(true)));
+ assertEquals("vegetables", Iterables.getOnlyElement(parsedSchema.getFieldNames()));
+ Schema.Field field = parsedSchema.getField("vegetables");
+
+ // test using equivalency, which permits out-of-orderness
+ assertTrue(
+ field
+ .getType()
+ .equivalent(
+ Schema.FieldType.array(
+ Schema.FieldType.row(
+ Schema.of(
+ Schema.Field.of("veggieName", Schema.FieldType.STRING),
Review Comment:
Do we want to keep the test checking that these first two fields occur in this order (which I think is guaranteed since they're required). We only want to relax the ordering of the subfields in "origin".
--
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 json schema test [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35597:
URL: https://github.com/apache/beam/pull/35597#discussion_r2208187808
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JsonSchemaConversionTest.java:
##########
@@ -285,24 +287,31 @@ public void testArrayWithNestedRefsBeamSchema() throws IOException {
String stringJsonSchema = new String(ByteStreams.toByteArray(inputStream), "UTF-8");
Schema parsedSchema = JsonUtils.beamSchemaFromJsonSchema(stringJsonSchema);
- assertThat(parsedSchema.getFieldNames(), containsInAnyOrder("vegetables"));
- assertThat(
- parsedSchema.getFields().stream().map(Schema.Field::getType).collect(Collectors.toList()),
- containsInAnyOrder(
- Schema.FieldType.array(
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.of("veggieName", Schema.FieldType.STRING),
- Schema.Field.of("veggieLike", Schema.FieldType.BOOLEAN),
- Schema.Field.nullable(
- "origin",
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.nullable("country", Schema.FieldType.STRING),
- Schema.Field.nullable("town", Schema.FieldType.STRING),
- Schema.Field.nullable(
- "region", Schema.FieldType.STRING)))))))
- .withNullable(true)));
+ assertEquals("vegetables", Iterables.getOnlyElement(parsedSchema.getFieldNames()));
+ Schema.Field field = parsedSchema.getField("vegetables");
+
+ // test using equivalency, which permits out-of-orderness
+ assertTrue(
+ field
+ .getType()
+ .equivalent(
+ Schema.FieldType.array(
+ Schema.FieldType.row(
+ Schema.of(
+ Schema.Field.of("veggieName", Schema.FieldType.STRING),
Review Comment:
Good point. added a check for top-level ordering
--
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 json schema test [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #35597:
URL: https://github.com/apache/beam/pull/35597#issuecomment-3074567254
R: @acrites
--
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 json schema test [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35597:
URL: https://github.com/apache/beam/pull/35597#issuecomment-3074571760
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
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 json schema test [beam]
Posted by "acrites (via GitHub)" <gi...@apache.org>.
acrites commented on code in PR #35597:
URL: https://github.com/apache/beam/pull/35597#discussion_r2208216488
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JsonSchemaConversionTest.java:
##########
@@ -285,24 +288,36 @@ public void testArrayWithNestedRefsBeamSchema() throws IOException {
String stringJsonSchema = new String(ByteStreams.toByteArray(inputStream), "UTF-8");
Schema parsedSchema = JsonUtils.beamSchemaFromJsonSchema(stringJsonSchema);
- assertThat(parsedSchema.getFieldNames(), containsInAnyOrder("vegetables"));
- assertThat(
- parsedSchema.getFields().stream().map(Schema.Field::getType).collect(Collectors.toList()),
- containsInAnyOrder(
- Schema.FieldType.array(
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.of("veggieName", Schema.FieldType.STRING),
- Schema.Field.of("veggieLike", Schema.FieldType.BOOLEAN),
- Schema.Field.nullable(
- "origin",
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.nullable("country", Schema.FieldType.STRING),
- Schema.Field.nullable("town", Schema.FieldType.STRING),
- Schema.Field.nullable(
- "region", Schema.FieldType.STRING)))))))
- .withNullable(true)));
+ assertEquals("vegetables", Iterables.getOnlyElement(parsedSchema.getFieldNames()));
+ Schema.Field field = parsedSchema.getField("vegetables");
+
+ // 2 out of 3 top-level fields are required, so ordering should be preserved on that level
Review Comment:
nit: comments should generally follow proper grammar, e.g. start with capital letters and end in punctuation. (Here and in the one below).
--
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 json schema test [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #35597:
URL: https://github.com/apache/beam/pull/35597#discussion_r2208225810
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JsonSchemaConversionTest.java:
##########
@@ -285,24 +288,36 @@ public void testArrayWithNestedRefsBeamSchema() throws IOException {
String stringJsonSchema = new String(ByteStreams.toByteArray(inputStream), "UTF-8");
Schema parsedSchema = JsonUtils.beamSchemaFromJsonSchema(stringJsonSchema);
- assertThat(parsedSchema.getFieldNames(), containsInAnyOrder("vegetables"));
- assertThat(
- parsedSchema.getFields().stream().map(Schema.Field::getType).collect(Collectors.toList()),
- containsInAnyOrder(
- Schema.FieldType.array(
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.of("veggieName", Schema.FieldType.STRING),
- Schema.Field.of("veggieLike", Schema.FieldType.BOOLEAN),
- Schema.Field.nullable(
- "origin",
- Schema.FieldType.row(
- Schema.of(
- Schema.Field.nullable("country", Schema.FieldType.STRING),
- Schema.Field.nullable("town", Schema.FieldType.STRING),
- Schema.Field.nullable(
- "region", Schema.FieldType.STRING)))))))
- .withNullable(true)));
+ assertEquals("vegetables", Iterables.getOnlyElement(parsedSchema.getFieldNames()));
+ Schema.Field field = parsedSchema.getField("vegetables");
+
+ // 2 out of 3 top-level fields are required, so ordering should be preserved on that level
Review Comment:
Done :)
--
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