GitHub PullRequestReviewCommentEvent at 2014-06-27 01:11:10
Repository: scalding
Actor: johnynek
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/twitter/scalding/pulls/comments/14276041","id":14276041,"diff_hunk":"@@ -108,5 +108,46 @@ class ShellObj[T](obj: T) {\n     run()\n     TypedTsv[R](\"item\").toIterator.toList\n   }\n+\n }\n \n+/**\n+ * Enrichment on TypedPipes allowing them to be run locally, independent of the overall flow.\n+ * @param pipe to wrap\n+ */\n+class ShellTypedPipe[T](pipe: TypedPipe[T]) extends ShellObj[TypedPipe[T]](pipe) {\n+\n+  /**\n+   * Shorthand for .write(dest).run\n+   */\n+  def save(dest: TypedSink[T] with Mappable[T]): TypedPipe[T] = {\n+\n+    val thisPipe = pipe.toPipe(dest.sinkFields)(dest.setter)\n+\n+    dest.writeFrom(thisPipe)","path":"scalding-repl/src/main/scala/com/twitter/scalding/ShellPipe.scala","position":67,"original_position":67,"commit_id":"36e55d9e4bc4f59c47ca57f958efb11029948cfe","original_commit_id":"36e55d9e4bc4f59c47ca57f958efb11029948cfe","user":{"login":"johnynek","id":67958,"avatar_url":"https://avatars.githubusercontent.com/u/67958?","gravatar_id":"fa94a62940f16a371fdd3e6e8e12c3a1","url":"https://api.github.com/users/johnynek","html_url":"https://github.com/johnynek","followers_url":"https://api.github.com/users/johnynek/followers","following_url":"https://api.github.com/users/johnynek/following{/other_user}","gists_url":"https://api.github.com/users/johnynek/gists{/gist_id}","starred_url":"https://api.github.com/users/johnynek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/johnynek/subscriptions","organizations_url":"https://api.github.com/users/johnynek/orgs","repos_url":"https://api.github.com/users/johnynek/repos","events_url":"https://api.github.com/users/johnynek/events{/privacy}","received_events_url":"https://api.github.com/users/johnynek/received_events","type":"User","site_admin":false},"body":"actually, I think if change the logic as I describe you can remove this:\r\n\r\n1) make localizedFlow work even on non-tail pipes (just walk walk up from thisPipe)\r\n2) pass the localizedFlow in manually:\r\n```scala\r\nval localFD = thisPipe.localizedFlow\r\ndest.writeFrom(thisPipe)(localFD, implicitly[Mode])\r\n// Now, dest still has not been used in the current flow.\r\nTypedPipe.from(dest)\r\n```","created_at":"2014-06-27T01:11:10Z","updated_at":"2014-06-27T01:11:10Z","html_url":"https://github.com/twitter/scalding/pull/918#discussion_r14276041","pull_request_url":"https://api.github.com/repos/twitter/scalding/pulls/918","_links":{"self":{"href":"https://api.github.com/repos/twitter/scalding/pulls/comments/14276041"},"html":{"href":"https://github.com/twitter/scalding/pull/918#discussion_r14276041"},"pull_request":{"href":"https://api.github.com/repos/twitter/scalding/pulls/918"}}},"pull_request":{"url":"https://api.github.com/repos/twitter/scalding/pulls/918","id":17662358,"html_url":"https://github.com/twitter/scalding/pull/918","diff_url":"https://github.com/twitter/scalding/pull/918.diff","patch_url":"https://github.com/twitter/scalding/pull/918.patch","issue_url":"https://api.github.com/repos/twitter/scalding/issues/918","number":918,"state":"open","title":"Snapshot a pipe in the REPL","user":{"login":"bholt","id":398411,"avatar_url":"https://avatars.githubusercontent.com/u/398411?","gravatar_id":"2cb04163c350dd11e7a08cc85e9641fe","url":"https://api.github.com/users/bholt","html_url":"https://github.com/bholt","followers_url":"https://api.github.com/users/bholt/followers","following_url":"https://api.github.com/users/bholt/following{/other_user}","gists_url":"https://api.github.com/users/bholt/gists{/gist_id}","starred_url":"https://api.github.com/users/bholt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bholt/subscriptions","organizations_url":"https://api.github.com/users/bholt/orgs","repos_url":"https://api.github.com/users/bholt/repos","events_url":"https://api.github.com/users/bholt/events{/privacy}","received_events_url":"https://api.github.com/users/bholt/received_events","type":"User","site_admin":false},"body":"**Do not merge yet: feedback needed, work in progress.**\r\n\r\nFixes #879, could be in the territory of #889.\r\n\r\nThis pull request attempts to address the issue with not being able to run sub-flows in the repl. Rather than resetting the implicit `flowDef` (which disallows re-running pipes -- #879), this adds the ability to run a \"sub-flow\" for a given TypedPipe that includes only the upstream pipes and sources needed to compute that tail pipe.\r\n\r\nThis is accessible in the REPL via a couple new enrichments to TypedPipe:\r\n- `.snapshot`: immediately runs and writes the pipe to a temporary SequenceFile, returning a TypedPipe with the SequenceFile as a source.\r\n- `.save(dest)`: same as snapshot but with a user-provided Sink (kinda like shorthand for `.write(dest).run`), also returning a TypedPipe with the saved file as a new Source.\r\n\r\nIn my current way of thinking, snapshots allow one to inspect intermediate steps of a job as they build it:\r\n\r\n~~~scala\r\n> val input = TypedPipe.from(TextLine(\"tutorial/data/hello.txt\"))\r\n> val wordsPipe: TypedPipe[String] = input.flatMap(_.split(\"\\\\s+\"))\r\n> val s1: TypedPipe[String] = wordsPipe.snapshot\r\n~~~\r\n\r\nOne then has a choice, when continuing to work on a job, to use the snapshot pipe, if they wish to \"memoize\" the intermediate work, or use the original pipe, which will cause the flow to be re-evaluated.\r\n\r\nBefore continuing to work on this, I wanted to elicit some feedback on the intended direction. Overall, I'm hoping to come up with behavior that is least surprising to users of Scalding, but also cater to users coming from other ad-hoc analytics backgrounds (i.e. Pig, Spark)\r\n\r\n1. Part of using snapshots should be good ways to actually *view* the contents of the snapshot. I would like to allow a `toList` kind of behavior that would read the contents into memory. This could either be implicit (`.toList` calls `.snapshot` internally), or `.snapshot` could return a new kind of TypedPipe that allowed `.toList` to be called on it. (#709 may be related -- I believe this \"sub-flow\" ability may fix one of the outstanding issues there)\r\n  - There could also be a Pig-like `dump` call added to snapshots that would just print the contents to stdout (though `.toList` pretty much subsumes this, as you can print a list to stdout)\r\n\r\n2. What should the semantics of `run` be in the context of this change? Currently it is broken because all the snapshots add to the implicit flowDef, causing `run` on the whole flow to balk at all the unconnected sources and sinks. Should `run` be fixed by just discarding the extraneous sources and sinks created in the REPL? Or should `run` just go away, in favor of always calling something like `save` to commit the contents of a pipe to a single Sink?\r\n\r\n3. Should `snapshot` ultimately be completely hidden from the user? This iteration allows an explicit choice to be made between building off of a snapshot of intermediate results or re-running the whole flow. An alternative would be to only create snapshots when people request to see the contents (via `toList`), and then swap out the memoized pipe for the original so the user gets consistent results. This leads to questions about whether memoized pipes should ever be invalidated when the original contents have changed (which sounds really daunting).\r\n\r\nThoughts?","created_at":"2014-06-26T17:51:34Z","updated_at":"2014-06-27T01:11:10Z","closed_at":null,"merged_at":null,"merge_commit_sha":"e88443916d82f9d39e94756824e4d324cc68ace3","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/twitter/scalding/pulls/918/commits","review_comments_url":"https://api.github.com/repos/twitter/scalding/pulls/918/comments","review_comment_url":"https://api.github.com/repos/twitter/scalding/pulls/comments/{number}","comments_url":"https://api.github.com/repos/twitter/scalding/issues/918/comments","statuses_url":"https://api.github.com/repos/twitter/scalding/statuses/36e55d9e4bc4f59c47ca57f958efb11029948cfe","head":{"label":"bholt:repl","ref":"repl","sha":"36e55d9e4bc4f59c47ca57f958efb11029948cfe","user":{"login":"bholt","id":398411,"avatar_url":"https://avatars.githubusercontent.com/u/398411?","gravatar_id":"2cb04163c350dd11e7a08cc85e9641fe","url":"https://api.github.com/users/bholt","html_url":"https://github.com/bholt","followers_url":"https://api.github.com/users/bholt/followers","following_url":"https://api.github.com/users/bholt/following{/other_user}","gists_url":"https://api.github.com/users/bholt/gists{/gist_id}","starred_url":"https://api.github.com/users/bholt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bholt/subscriptions","organizations_url":"https://api.github.com/users/bholt/orgs","repos_url":"https://api.github.com/users/bholt/repos","events_url":"https://api.github.com/users/bholt/events{/privacy}","received_events_url":"https://api.github.com/users/bholt/received_events","type":"User","site_admin":false},"repo":{"id":20580226,"name":"scalding","full_name":"bholt/scalding","owner":{"login":"bholt","id":398411,"avatar_url":"https://avatars.githubusercontent.com/u/398411?","gravatar_id":"2cb04163c350dd11e7a08cc85e9641fe","url":"https://api.github.com/users/bholt","html_url":"https://github.com/bholt","followers_url":"https://api.github.com/users/bholt/followers","following_url":"https://api.github.com/users/bholt/following{/other_user}","gists_url":"https://api.github.com/users/bholt/gists{/gist_id}","starred_url":"https://api.github.com/users/bholt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bholt/subscriptions","organizations_url":"https://api.github.com/users/bholt/orgs","repos_url":"https://api.github.com/users/bholt/repos","events_url":"https://api.github.com/users/bholt/events{/privacy}","received_events_url":"https://api.github.com/users/bholt/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/bholt/scalding","description":"A Scala API for Cascading","fork":true,"url":"https://api.github.com/repos/bholt/scalding","forks_url":"https://api.github.com/repos/bholt/scalding/forks","keys_url":"https://api.github.com/repos/bholt/scalding/keys{/key_id}","collaborators_url":"https://api.github.com/repos/bholt/scalding/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/bholt/scalding/teams","hooks_url":"https://api.github.com/repos/bholt/scalding/hooks","issue_events_url":"https://api.github.com/repos/bholt/scalding/issues/events{/number}","events_url":"https://api.github.com/repos/bholt/scalding/events","assignees_url":"https://api.github.com/repos/bholt/scalding/assignees{/user}","branches_url":"https://api.github.com/repos/bholt/scalding/branches{/branch}","tags_url":"https://api.github.com/repos/bholt/scalding/tags","blobs_url":"https://api.github.com/repos/bholt/scalding/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/bholt/scalding/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/bholt/scalding/git/refs{/sha}","trees_url":"https://api.github.com/repos/bholt/scalding/git/trees{/sha}","statuses_url":"https://api.github.com/repos/bholt/scalding/statuses/{sha}","languages_url":"https://api.github.com/repos/bholt/scalding/languages","stargazers_url":"https://api.github.com/repos/bholt/scalding/stargazers","contributors_url":"https://api.github.com/repos/bholt/scalding/contributors","subscribers_url":"https://api.github.com/repos/bholt/scalding/subscribers","subscription_url":"https://api.github.com/repos/bholt/scalding/subscription","commits_url":"https://api.github.com/repos/bholt/scalding/commits{/sha}","git_commits_url":"https://api.github.com/repos/bholt/scalding/git/commits{/sha}","comments_url":"https://api.github.com/repos/bholt/scalding/comments{/number}","issue_comment_url":"https://api.github.com/repos/bholt/scalding/issues/comments/{number}","contents_url":"https://api.github.com/repos/bholt/scalding/contents/{+path}","compare_url":"https://api.github.com/repos/bholt/scalding/compare/{base}...{head}","merges_url":"https://api.github.com/repos/bholt/scalding/merges","archive_url":"https://api.github.com/repos/bholt/scalding/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/bholt/scalding/downloads","issues_url":"https://api.github.com/repos/bholt/scalding/issues{/number}","pulls_url":"https://api.github.com/repos/bholt/scalding/pulls{/number}","milestones_url":"https://api.github.com/repos/bholt/scalding/milestones{/number}","notifications_url":"https://api.github.com/repos/bholt/scalding/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/bholt/scalding/labels{/name}","releases_url":"https://api.github.com/repos/bholt/scalding/releases{/id}","created_at":"2014-06-06T22:41:22Z","updated_at":"2014-06-25T01:53:32Z","pushed_at":"2014-06-26T22:48:46Z","git_url":"git://github.com/bholt/scalding.git","ssh_url":"git@github.com:bholt/scalding.git","clone_url":"https://github.com/bholt/scalding.git","svn_url":"https://github.com/bholt/scalding","homepage":"http://twitter.com/scalding","size":6851,"stargazers_count":0,"watchers_count":0,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"develop"}},"base":{"label":"twitter:develop","ref":"develop","sha":"9f3582376b62512cd8b5476a92236a0abbf15ae0","user":{"login":"twitter","id":50278,"avatar_url":"https://avatars.githubusercontent.com/u/50278?","gravatar_id":"2f4a8254d032a8ec5e4c48d461e54fcc","url":"https://api.github.com/users/twitter","html_url":"https://github.com/twitter","followers_url":"https://api.github.com/users/twitter/followers","following_url":"https://api.github.com/users/twitter/following{/other_user}","gists_url":"https://api.github.com/users/twitter/gists{/gist_id}","starred_url":"https://api.github.com/users/twitter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twitter/subscriptions","organizations_url":"https://api.github.com/users/twitter/orgs","repos_url":"https://api.github.com/users/twitter/repos","events_url":"https://api.github.com/users/twitter/events{/privacy}","received_events_url":"https://api.github.com/users/twitter/received_events","type":"Organization","site_admin":false},"repo":{"id":3146558,"name":"scalding","full_name":"twitter/scalding","owner":{"login":"twitter","id":50278,"avatar_url":"https://avatars.githubusercontent.com/u/50278?","gravatar_id":"2f4a8254d032a8ec5e4c48d461e54fcc","url":"https://api.github.com/users/twitter","html_url":"https://github.com/twitter","followers_url":"https://api.github.com/users/twitter/followers","following_url":"https://api.github.com/users/twitter/following{/other_user}","gists_url":"https://api.github.com/users/twitter/gists{/gist_id}","starred_url":"https://api.github.com/users/twitter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twitter/subscriptions","organizations_url":"https://api.github.com/users/twitter/orgs","repos_url":"https://api.github.com/users/twitter/repos","events_url":"https://api.github.com/users/twitter/events{/privacy}","received_events_url":"https://api.github.com/users/twitter/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/twitter/scalding","description":"A Scala API for Cascading","fork":false,"url":"https://api.github.com/repos/twitter/scalding","forks_url":"https://api.github.com/repos/twitter/scalding/forks","keys_url":"https://api.github.com/repos/twitter/scalding/keys{/key_id}","collaborators_url":"https://api.github.com/repos/twitter/scalding/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/twitter/scalding/teams","hooks_url":"https://api.github.com/repos/twitter/scalding/hooks","issue_events_url":"https://api.github.com/repos/twitter/scalding/issues/events{/number}","events_url":"https://api.github.com/repos/twitter/scalding/events","assignees_url":"https://api.github.com/repos/twitter/scalding/assignees{/user}","branches_url":"https://api.github.com/repos/twitter/scalding/branches{/branch}","tags_url":"https://api.github.com/repos/twitter/scalding/tags","blobs_url":"https://api.github.com/repos/twitter/scalding/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/twitter/scalding/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/twitter/scalding/git/refs{/sha}","trees_url":"https://api.github.com/repos/twitter/scalding/git/trees{/sha}","statuses_url":"https://api.github.com/repos/twitter/scalding/statuses/{sha}","languages_url":"https://api.github.com/repos/twitter/scalding/languages","stargazers_url":"https://api.github.com/repos/twitter/scalding/stargazers","contributors_url":"https://api.github.com/repos/twitter/scalding/contributors","subscribers_url":"https://api.github.com/repos/twitter/scalding/subscribers","subscription_url":"https://api.github.com/repos/twitter/scalding/subscription","commits_url":"https://api.github.com/repos/twitter/scalding/commits{/sha}","git_commits_url":"https://api.github.com/repos/twitter/scalding/git/commits{/sha}","comments_url":"https://api.github.com/repos/twitter/scalding/comments{/number}","issue_comment_url":"https://api.github.com/repos/twitter/scalding/issues/comments/{number}","contents_url":"https://api.github.com/repos/twitter/scalding/contents/{+path}","compare_url":"https://api.github.com/repos/twitter/scalding/compare/{base}...{head}","merges_url":"https://api.github.com/repos/twitter/scalding/merges","archive_url":"https://api.github.com/repos/twitter/scalding/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/twitter/scalding/downloads","issues_url":"https://api.github.com/repos/twitter/scalding/issues{/number}","pulls_url":"https://api.github.com/repos/twitter/scalding/pulls{/number}","milestones_url":"https://api.github.com/repos/twitter/scalding/milestones{/number}","notifications_url":"https://api.github.com/repos/twitter/scalding/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/twitter/scalding/labels{/name}","releases_url":"https://api.github.com/repos/twitter/scalding/releases{/id}","created_at":"2012-01-10T16:22:08Z","updated_at":"2014-06-26T07:38:16Z","pushed_at":"2014-06-26T23:59:25Z","git_url":"git://github.com/twitter/scalding.git","ssh_url":"git@github.com:twitter/scalding.git","clone_url":"https://github.com/twitter/scalding.git","svn_url":"https://github.com/twitter/scalding","homepage":"http://twitter.com/scalding","size":22129,"stargazers_count":1588,"watchers_count":1588,"language":"Scala","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":360,"mirror_url":null,"open_issues_count":153,"forks":360,"open_issues":153,"watchers":1588,"default_branch":"develop"}},"_links":{"self":{"href":"https://api.github.com/repos/twitter/scalding/pulls/918"},"html":{"href":"https://github.com/twitter/scalding/pull/918"},"issue":{"href":"https://api.github.com/repos/twitter/scalding/issues/918"},"comments":{"href":"https://api.github.com/repos/twitter/scalding/issues/918/comments"},"review_comments":{"href":"https://api.github.com/repos/twitter/scalding/pulls/918/comments"},"review_comment":{"href":"https://api.github.com/repos/twitter/scalding/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/twitter/scalding/pulls/918/commits"},"statuses":{"href":"https://api.github.com/repos/twitter/scalding/statuses/36e55d9e4bc4f59c47ca57f958efb11029948cfe"}}}}