Skip to content

/ hail

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hail] [streams] more complicated streams #7229

Merged
merged 25 commits into from Dec 5, 2019
Merged

[hail] [streams] more complicated streams #7229

merged 25 commits into from Dec 5, 2019

Conversation

@iitalics
Copy link
Contributor

iitalics commented Oct 8, 2019 •
edited

Stacked on #7228 (transitively, #7207)

Implements the following pull based streams:

  • ArrayFlatMap
  • ArrayLeftJoinDistinct
  • ArrayScan
  • ArrayAggScan
@iitalics iitalics force-pushed the iitalics:jvm-streams branch 2 times, most recently from 7d35043 to 5e6db04 Oct 9, 2019
@tpoterba

This comment has been minimized.

Sign in to view
Copy link
Collaborator

tpoterba commented Oct 10, 2019

nice work, Milo!
@iitalics iitalics force-pushed the iitalics:jvm-streams branch from 5e6db04 to 76e57cf Oct 11, 2019
@patrick-schultz
patrick-schultz requested changes Oct 29, 2019
View changes
Copy link
Collaborator

patrick-schultz left a comment

This is really great. I'm excited to get it merged and start using it!

A few questions and requests while I finish working through it:
hail/src/main/scala/is/hail/expr/ir/EmitStream.scala Outdated Show resolved Hide resolved
hail/src/main/scala/is/hail/asm4s/joinpoint/ParameterPack.scala Outdated Show resolved Hide resolved
hail/src/main/scala/is/hail/asm4s/joinpoint/ParameterPack.scala Outdated Show resolved Hide resolved
hail/src/main/scala/is/hail/expr/ir/EmitStream.scala Show resolved Hide resolved
hail/src/main/scala/is/hail/expr/ir/EmitStream.scala Outdated Show resolved Hide resolved
@patrick-schultz
patrick-schultz reviewed Oct 30, 2019
View changes
hail/src/test/scala/is/hail/expr/ir/EmitStreamSuite.scala Outdated Show resolved Hide resolved
@iitalics iitalics force-pushed the iitalics:jvm-streams branch from 5a10ab8 to 3a1c1b2 Oct 30, 2019
@patrick-schultz

This comment has been minimized.

Sign in to view
Copy link
Collaborator

patrick-schultz commented Nov 5, 2019

This isn't WIP anymore, is it? Also, do you just want to delete the first stream PR? It's gotten a bit out of sync with the changes in this one. If you're happy with this, I'm ready to approve.
@iitalics iitalics force-pushed the iitalics:jvm-streams branch from e4f5e6d to 7c41579 Nov 8, 2019
@tpoterba

This comment has been minimized.

Sign in to view
Copy link
Collaborator

tpoterba commented Dec 5, 2019

What do we need to do to get this in?
iitalics added 14 commits Oct 8, 2019
@iitalics
some more ParameterPack utilities
f6a47ad
@iitalics
create EmitStream interface; implements ArrayRange + ToStream + NA
1d8f8e4 
standalone emitstream missing impl

make NA case trivially easy

simplify range impl
@iitalics
EmitStream test suite
c0313fb 
fix the Let test
@iitalics
stream impl of ArrayMap, ArrayFilter
4277de1 
add Skip

other streams

add tests

delete all but map/filter

delete tests
@iitalics
stream impl of ArrayMake
e810649 
simplify makeArray implementation
@iitalics
EmitStream tests for map, filter
37a21a9
@iitalics
EmitStream tests for make
f7a44bc
@iitalics
remove Empty in favor of "empty state" that gives an empty stream
7ee6968 
delete Empty; rename dummyState => emptyState

get rid of Empty in toArrayIterator

 dummyState to emptyState in missing/range

get rid of Empty in test suite

delete Empty in map, filter

rename dummyState in make
@iitalics
sequence.length fix
7904e41
@iitalics
stream impl of ArrayFlatMap
24f85fa 
leftjoin test

clean up compose impl

less code dup in EmitStream.compose by doing Skip(..dummyState..)

flatmap new emptyState version
@iitalics
TypedTriplet utility for ParameterPacking values associated w PType
7c112f4
@iitalics
stream impl of ArrayLeftJoinDistinct
7141136 
leftjoin new emptyState version
@iitalics
EmitStream tests for flatMap
8b298eb 
get rid of Empty in compose

get rid of empty in compose (2)
@iitalics
EmitStream tests for leftJoinDistinct
d0e30fc 
get rid of Empty in leftjoin
iitalics added 11 commits Oct 8, 2019
@iitalics
stream impl of Array(Agg)Scan
b98675e 
add all but map/filter

add tests

more complex let test

...more

get rid of Empty in scan
@iitalics
EmitStream tests for scan, aggScan
08cbcd4
@iitalics
more type annotations
d5f1e45
@iitalics
use HailSuite instead of TestNGSuite
2179564
@iitalics
rename ParameterPack."localMemoize" => "let"
5053d98
@iitalics
make TypedTriplet constructor private
abb4e03
@iitalics
guardParam (sort of like contramap) to simplify emitStream
923998d
@iitalics
remove Skip()
e7160b9
@iitalics
avoid calling some continuations more than once
1a7460c 
avoid calling the continuation too many times in EmitStream.sequence

avoid calling the Missing continuation in guardParam
@iitalics
omit 'canGenerateLiterals' since it was deleted in prev commit
cad54c8
@iitalics
fix ArrayAggScan again by making ExecuteContext's
Loading status checks…
853d491
@iitalics iitalics force-pushed the iitalics:jvm-streams branch from 7c41579 to 853d491 Dec 5, 2019
@iitalics

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

iitalics commented Dec 5, 2019

just rebased and fixed something. should be ready to merge after it passes CI
@patrick-schultz
patrick-schultz approved these changes Dec 5, 2019
View changes
@tpoterba

This comment has been minimized.

Sign in to view
Copy link
Collaborator

tpoterba commented Dec 5, 2019

😱 🎉
@danking danking merged commit 9f2e22e into hail-is:master Dec 5, 2019
1 check passed
1 check passed
ci-test success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrick-schultz patrick-schultz

Assignees
No one assigned
Labels
infrastructure
Projects
None yet
Milestone
No milestone
4 participants
@iitalics @tpoterba @patrick-schultz @danking
You can’t perform that action at this time.