Uh oh!
There was an error while loading. Please reload this page.
[BEAM-121] Add DisplayData for combine transforms - #126
Conversation
swegner
commented
Apr 5, 2016
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.
kennknowles
commented
Apr 8, 2016
The conflicts prevent test run. Probably want to rebase and push to kick them. |
c6edc08 to
40b8abdCompare7dcf68a to
fcdc0c3Compareswegner
commented
Apr 14, 2016
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).
bjchambers
commented
Apr 14, 2016
A few minor comments, then LGTM. |
I've addressed all feedback so far. Please take another look. @bjchambers |
bjchambers
commented
Apr 15, 2016
LGTM |
swegner
commented
May 16, 2016
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
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.