[BEAM-121] Add display data to ParDo transforms #125
Conversation
|
R: @bjchambers |
|
|
||
| @Override | ||
| public void populateDisplayData(DisplayData.Builder builder) { | ||
| Filter.populateDisplayData(builder, String.format("x ≤ %s", value)); |
There was a problem hiding this comment.
@davorbonaci for comments on using unicode in the codebase. I'm not sure if we should care.
There was a problem hiding this comment.
Note also this'll get serialized as JSON and passed into the service. I don't see any reason we shouldn't support Unicode..
|
Conflicts blocking test run(s). |
| Bound<String, String> parDo = ParDo.of(fn); | ||
|
|
||
| DisplayData displayData = DisplayData.from(parDo); | ||
| assertThat(displayData, includes(fn)); |
There was a problem hiding this comment.
Maybe add some messages here? Every time I see these tests, I expect to see something like "and make sure the fnMetadata" is there, then I need to find this and parse it.
assertThat("Copies over all data from the function", displayData, includes(fn));
Might make it easier to parse the assertions.
There was a problem hiding this comment.
I see where you're coming from. I'd prefer to make the Matcher method self-describing rather than require a comment on each usage. Maybe something like:
assertThat(displayData, includesDisplayDataFrom(fn));
Let me take this as a follow-up item once the pending PRs go through in order to avoid merge conflicts. Tracking in BEAM-195
swegner
force-pushed
the
displaydata-pardo
branch
2 times, most recently
from
e71de00 to
a01229b
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
swegner
force-pushed
the
displaydata-pardo
branch
from
a01229b to
fa16dc8
Compare
| .build() | ||
| ); | ||
|
|
||
| ImmutableSet<ImmutableMap<String, Object>> expectedFn2DisplayData = ImmutableSet.of( |
There was a problem hiding this comment.
This shouldn't need to change anymore -- the Matcher based version should work, right?
There was a problem hiding this comment.
This is the version that existed in my branch before mergepocolypse. It includes a few more fields which are now being serialized, and is also a stronger assertion in that it is testing for set equality rather than subset.
|
LGTM. |
|
Backported via GoogleCloudPlatform/DataflowJavaSDK#216 |
#! Stabilize sort-test
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
number, if there is one.
Individual Contributor License Agreement.