[BEAM-121] Add DisplayData for combine transforms#126
[BEAM-121] Add DisplayData for combine transforms#126swegner wants to merge 13 commits intoapache:masterfrom
Conversation
|
R: @bjchambers |
| public void populateDisplayData(DisplayData.Builder builder) { | ||
| builder | ||
| .add("numQuantiles", numQuantiles) | ||
| .add("comparer", compareFn.getClass()); |
There was a problem hiding this comment.
I'm wondering if we want to also use the compareFn.toString(). I'm thinking about cases where a user may have constructed a comparator by wrapping one or more actual comparators. In that case, just knowing the class of the outer comparator may not be complete information. Also happy to defer until this becomes a problem.
There was a problem hiding this comment.
Let's defer until this is an issue. In most cases comparators won't implement toString(), so the additional display data would be redundant and not as nicely formatted as compareFn.getClass(). I'd rather give an explicit was for comparators to register display data.
Perhaps we could inspect (compareFn instanceof HasDisplayData)-- but, let's save that until we have a use-case.
|
The conflicts prevent test run. Probably want to rebase and push to kick them. |
swegner
force-pushed
the
displaydata-combine
branch
from
c6edc08 to
40b8abd
Compare
swegner
force-pushed
the
displaydata-combine
branch
from
7dcf68a to
fcdc0c3
Compare
|
I've addressed all feedback so far. Please take another look. @bjchambers |
| */ | ||
| private final long sampleSize; | ||
|
|
||
| /** |
There was a problem hiding this comment.
also, the comma seems unnecessary
| String baseNamespace = combineFnEntries.getKey().getName(); | ||
| for (int i = 0; i < combineFns.size(); i++) { | ||
| HasDisplayData combineFn = combineFns.get(i); | ||
| String namespace = String.format("%s$%d", baseNamespace, i + 1); |
There was a problem hiding this comment.
Not sure it matters, but we may want to use something other than $, since that is already used for anonymous inner classes. Maybe #? Although at that point, we've already gone to the point of having the path (in this case the path is just a number).
|
A few minor comments, then LGTM. |
|
I've addressed all feedback so far. Please take another look. @bjchambers |
|
LGTM |
|
Backported via GoogleCloudPlatform/DataflowJavaSDK#216 |
…-from-reduce-by-key apache#121 allow multiple elements from reduce by key
* feat: create AsyncIter class for mocking * fix: type error on mocked return on batch_get_documents
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.