Skip to content
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] Simplify nested struct inserts broken by lets #7096

Merged
merged 3 commits into from Sep 20, 2019

Conversation

@tpoterba
Copy link
Collaborator

commented Sep 20, 2019

Following up on the last PR, that didn't actually solve the general problem. See the test at the bottom for an example of an IR that wasn't matching the rule.

case _ => true
}
case Let(name, value, body) if {
@tailrec def getNestedInsertFields(x: IR): Option[InsertFields] = x match {

This comment has been minimized.

Copy link
@patrick-schultz

patrick-schultz Sep 20, 2019

Collaborator

Ultimately, I think the right way to handle optimization rules being obscured by lets (and other things like ifs) is with commuting conversions. In this case, the commuting conversion is Let("a", Let("b", B, A), body) -> Let("b", B, Let("a", A, body)). After all instances of that rule have run, the value of a let never contains a top-level let.

Do you think that would have any negative consequences in the current system? I'm a little concerned about how complex some of these simplify rules are getting.

This comment has been minimized.

Copy link
@tpoterba

tpoterba Sep 20, 2019

Author Collaborator

oh, this does seem better. I'll give that a shot.

This comment has been minimized.

Copy link
@tpoterba

tpoterba Sep 20, 2019

Author Collaborator

great suggestion! Takes the simplify.scala change down to one line :)

@tpoterba tpoterba force-pushed the tpoterba:struct-unpacking-2 branch from 28e3a82 to faa0d85 Sep 20, 2019
@danking danking merged commit edd724f into hail-is:master Sep 20, 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
Projects
None yet
3 participants
You can’t perform that action at this time.