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

Fix failing YAML mapping IT #32851

Merged
merged 2 commits into from
Oct 18, 2024
Merged

Fix failing YAML mapping IT #32851

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d363a17
Fix failing YAML mapping IT
Polber Oct 17, 2024
ef6caf4
Added a TODO.
robertwb Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sdks/python/apache_beam/yaml/tests/map.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ pipelines:
config:
append: true
fields:
named_field: element
literal_int: 10
named_field: element
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this order respected but the other ordering wasn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were in essence 2 issues (or 1 issue that required 2 fixes)

  1. Due to Schema inference parameterized types #32757 (for reasons I'm not entirely sure of), the ordering of the Row sent from the Java MapToFields transform was changed. To fix this, I reordered the expected output in the AssertEqual
  2. Because the change in 1) did not affect Python Rows, the AssertEqual would now fail for Python MapToFields since the order of the output Row did not change. To fix this, I changed the order of the fields in the config to reflect the new order in the AssertEqual

Ideally, the Java transform should not be changing the order (given that Python respects order, Java probably should too now that cross-language is so widespread), but I wanted to get tests green (and initially I thought this was a problem with the __eq__ being too restrictive with key order).

Perhaps more investigation should be done on #32757 to see why the ordering changed, or at the very least, I could open a bug about how Row ordering changes over Xlang.

Copy link
Contributor Author

@Polber Polber Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like there was a change to sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/providers/JavaRowUdf.java in that PR (more specifically StaticSchemaInference.fieldFromType) which likely messed with how the schema was parsed in Java MapToFields

Actually that is per field, so it is more likely something else more tied to how expansion service parses the Schema. I can look into it next week if it is still an issue

literal_float: 1.5
literal_str: '"abc"'

Expand All @@ -42,5 +42,5 @@ pipelines:
- type: AssertEqual
config:
elements:
- {element: 100, named_field: 100, literal_int: 10, literal_float: 1.5, literal_str: "abc"}
- {element: 200, named_field: 200, literal_int: 10, literal_float: 1.5, literal_str: "abc"}
- {element: 100, literal_int: 10, named_field: 100, literal_float: 1.5, literal_str: "abc"}
- {element: 200, literal_int: 10, named_field: 200, literal_float: 1.5, literal_str: "abc"}
Loading