Skip to content

[BEAM-3271] Improve Splittable ParDo translation#3282

Merged
asfgit merged 2 commits into
apache:masterfrom
kennknowles:Splittable-ParDo-translation
Jun 6, 2017
Merged

[BEAM-3271] Improve Splittable ParDo translation#3282
asfgit merged 2 commits into
apache:masterfrom
kennknowles:Splittable-ParDo-translation

Conversation

@kennknowles
Copy link
Copy Markdown
Member

@kennknowles kennknowles commented Jun 1, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

R: @tgroh another one peeled off, this one is for Splittable ParDo.

It is different than the others in that this transform is not designed to be serializable, as it is only a post-surgery artifact.

It brings up again the decision to make a unified pre/post serialization API. We could simply not have one if we follow this workflow:

  • serde the pipeline (now nodes have only RawPTransform)
  • perform surgery
  • serde the pipeline (RawPTransform "just works" and all "fresh" nodes from surgery now also contain RawPTransform)
  • run

Just musing. This latter world means that pseudo-primitives all need registered payloads. I don't totally love this, because they may contain a bunch of stuff specific to the language of the runner and it is just boilerplate to give them payloads.

@asfbot
Copy link
Copy Markdown

asfbot commented Jun 1, 2017

Build finished.
--none--

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.01%) to 70.676% when pulling 082fd5a on kennknowles:Splittable-ParDo-translation into eee6726 on apache:master.

@kennknowles kennknowles force-pushed the Splittable-ParDo-translation branch from 082fd5a to 616a07c Compare June 6, 2017 04:25
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.006%) to 70.611% when pulling 616a07c on kennknowles:Splittable-ParDo-translation into 6d64c6e on apache:master.

@kennknowles
Copy link
Copy Markdown
Member Author

Rebased on the latest master. This is probably easiest to review in two commits, though not so big either way.


@NewTracker
public RestrictionTracker<Integer> newTracker(Integer restriction) {
return null; // not actually nullable
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

throw new UnsupportedOperationException()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@kennknowles kennknowles force-pushed the Splittable-ParDo-translation branch from 616a07c to 1b00d95 Compare June 6, 2017 20:13
@asfgit asfgit merged commit 1b00d95 into apache:master Jun 6, 2017
asfgit pushed a commit that referenced this pull request Jun 6, 2017
  Improve Splittable ParDo translation
  Fix RawPTransform translation
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.005%) to 70.628% when pulling 1b00d95 on kennknowles:Splittable-ParDo-translation into 7c608c3 on apache:master.

@kennknowles kennknowles deleted the Splittable-ParDo-translation branch September 29, 2017 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants