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

Improve simplify #3429

Merged
merged 5 commits into from Apr 24, 2018

Conversation

Projects
None yet
2 participants
@cseed
Copy link
Collaborator

cseed commented Apr 24, 2018

remove unused let
push aggfilter into aggmap (if possible)
fuse aggmaps
Added ir.Binds (list of variables bound by an IR)
Added ir.Mentions (whether a variable is free in an IR)
propagate NA where strict
fuse ArrayMaps

Also improved ir.Pretty (more headers, was missing some closing parens).

This cleans up most of the small IR opportunities I've seen in @konradjk's scripts, although probably won't make a huge difference (except maybe pushing AggFilter into AggMap).

cseed added some commits Apr 23, 2018

Improve simplify.
remove unused let
push aggfilter into aggmap (if possible)
fuse aggmaps
Added ir.Binds (list of variables bound by an IR)
Added ir.Mentions (whether a variable is free in an IR)
more improvements
propagate NA where strict
fuse ArrayMaps
@@ -15,9 +15,6 @@ object FoldConstants {
throw new MatchError(ir)

ir match {
case If(NA(_), _, _) =>

This comment has been minimized.

@jbloom22

jbloom22 Apr 24, 2018

Collaborator

Did you mean to remove this? I see // If(NA, ...) handled by FoldConstants in Simplify

This comment has been minimized.

@cseed

cseed Apr 24, 2018

Collaborator

Comment was wrong (surprise). Deleted the comment.

def apply(ir: BaseIR): BaseIR = {
RewriteBottomUp(ir, matchErrorToNone {
// optimze IR
// optimize IR

This comment has been minimized.

@jbloom22

jbloom22 Apr 24, 2018

Collaborator

nice optimization :)


// propagate NA
case x: IR
if isStrict(x) && Children(x).exists(_.isInstanceOf[NA]) =>

This comment has been minimized.

@jbloom22

jbloom22 Apr 24, 2018

Collaborator

even at this length i'd prefer if on same line for parallel structure to other cases, but leave as is if you find this more readable

This comment has been minimized.

@cseed

cseed Apr 24, 2018

Collaborator

I don't care. I made it one line.

addressed comments

@cseed cseed merged commit 24923b8 into hail-is:master Apr 24, 2018

2 checks passed

tests-spark-2.0.2 Finished TeamCity Build Hail Source Code :: PRs Only :: Hail Test & Jar (Spark 2.0.2) [the build of record] : Running
Details
tests-spark-2.2.0 Finished TeamCity Build Hail Source Code :: PRs Only :: Hail Test & Jar (Spark 2.2.0) : Running
Details

konradjk added a commit to konradjk/hail that referenced this pull request Jun 12, 2018

Improve simplify (hail-is#3429)
* improve pretty

* Improve simplify.

remove unused let
push aggfilter into aggmap (if possible)
fuse aggmaps
Added ir.Binds (list of variables bound by an IR)
Added ir.Mentions (whether a variable is free in an IR)

* more improvements

propagate NA where strict
fuse ArrayMaps

* fixed compile error

* addressed comments

jackgoldsmith4 added a commit to jackgoldsmith4/hail that referenced this pull request Jun 25, 2018

Improve simplify (hail-is#3429)
* improve pretty

* Improve simplify.

remove unused let
push aggfilter into aggmap (if possible)
fuse aggmaps
Added ir.Binds (list of variables bound by an IR)
Added ir.Mentions (whether a variable is free in an IR)

* more improvements

propagate NA where strict
fuse ArrayMaps

* fixed compile error

* addressed comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment