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.

Sign up for GitHub

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

Jump to bottom

[BEAM-7210] Fix non-deterministic row access #8474

Merged
merged 1 commit into from May 6, 2019
Merged

[BEAM-7210] Fix non-deterministic row access #8474

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoSchemaTest.java
View file
Open in desktop
Expand Up @@ -388,7 +388,7 @@ public void testInferredSchemaPipeline() {
new DoFn<Inferred, String>() {
@ProcessElement
public void process(@Element Row row, OutputReceiver<String> r) {
r.output(row.getString(0) + ":" + row.getInt32(1));
r.output(row.getString("stringField") + ":" + row.getInt32("integerField"));
Copy link
Contributor

Choose a reason for hiding this comment

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

So basically it is not safe to use AutoValue with Schemas. Should we convert the example to use a Pojo instead (like it previously did)? Should we throw an error that schemas do not work deterministically in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is perfectly safe. What's not safe is assuming that the order of the schema fields will match the order of the methods in your class. In fact it's technically not safe for POJOs either, as AFAICT Java makes no guarantees there either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just wondered whether we should throw an error when users try to access fields via a non-deterministic position? It seems like this could cause subtle bugs when two fields have the same type.

}
}));
PAssert.that(output).containsInAnyOrder("a:1", "b:2", "c:3");
Expand Down