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

[query] support emit-level parameters #8371

Merged
merged 7 commits into from Apr 4, 2020
Merged

Conversation

cseed
Copy link
Collaborator

@cseed cseed commented Mar 27, 2020

It might not look like it, but I promise, I'm trying to keep these as small as possible.

Summary of changes:

  • EmitMethodBuilder, etc. no longer implement WrappedMethodBuilder, etc. but they do proxy the necessary functions.
  • Emit method params and return values are now statically either: an Code[T] with type TypeInfo[T], or an EmitCode, with type PType.
  • Added ParamType to wrap the two parameter type options, and Param to wrap the two code options, with implicit conversions.
  • A bunch of methods now have emit and code variants: getEmitParam, getCodeParam, invokeCode, invokeEmit (where code, emit refer to the return value).
  • I made the minimal changes to get things working again. Some code still code parameters to pass emit values. I will fix these in a later PR.
  • Where possible, make the class implemented by Compile concrete rather than generic. I think this can be pushed through the entire code base and we can remove the option to build generic methods. We should have done this a long time ago.
  • ModuleBuilder can now create type-specialized tuple types. This is used for EmitCode return values. I'm not sure if this is actually used yet.
  • Require RVDType rowType to be required. Require TypeValue global type to be required. Fix lots of places to make this true. In a few spots (e.g. TableMap{Rows, Globals}), I had to wrap the IR being compiled in a Coalesce with a Die to make sure the return type is required.
  • Cleaned up the dependent function interface to be closer to what we have now with MethodBuilder, etc. DependentFunctionBuilder is now just an apply_method: DependentMethodBuilder, EmitFunctionBuilder analogously. DependentMethodBuilder wraps a MethodBuilder, EmitMethodBuilder wraps a DependentMethodBuilder and an EmitMethodBuilder.
  • Add equality comparison to TypeInfo[_]
  • Add methods to convert IndexedSeq[Code[_]] to/from PCode and EmitCode. These are used to pass EmitCode as arguments to method invocation. If an emit parameter is required, the missingness boolean is omitted, otherwise it is present.

Furthermore, this change also adds requiredness to many things and improves ptype interfaces:

  • added PType.literalPType that infers PTypes from Scala literals, use in a few places (emit for Literal, BroadcastRegionValue constructor from annotation, etc.)
  • require Table global and row types to be required
  • same for MatrixValue, but also cols and entries (the entries array, not individual entries, which an be missing)
  • Don't upcast globals in TableKeyBy and TableOrderBy
  • added EType setRequired
  • AbstractCodecSpecs assert row and global etypes are present at the toplevel, and setRequired(true) if they are coming from encoders written by previous versions
  • rename PType.copyFromType to PType.copyFromAdddres. Modify it so it can "downcast": convert to a PType with greater requiredness. This is used in converting TableValues to MatrixValues to satisfy the requiredness assertions.

@cseed cseed changed the title [query] [query] support emit-level parameters Mar 27, 2020
@cseed cseed mentioned this pull request Mar 27, 2020
@cseed cseed removed the stacked PR label Apr 2, 2020
@cseed
Copy link
Collaborator Author

cseed commented Apr 2, 2020

The rebase was a little messy. I got it compiling, but there will probably be some minor issues with the tests. It was passing before, so I expect the overall code to be sound.

@cseed
Copy link
Collaborator Author

cseed commented Apr 2, 2020

Looks like some Python failures. It will take me a little while to track them down, but they all look minor and of two forms I understand:

  • RVDType being constructed with the row type being optional,
  • and incorrect type signatures when building emit methods due to mismatched missingness.
    Go ahead and review while I fix them.

Copy link
Collaborator

@chrisvittal chrisvittal left a comment

Choose a reason for hiding this comment

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

To my eye, this was super clean. A few nits. Substantive parts of the change all look great.

hail/build.gradle Outdated Show resolved Hide resolved
hail/src/main/scala/is/hail/lir/Emit.scala Outdated Show resolved Hide resolved
hail/src/test/scala/is/hail/expr/ir/PruneSuite.scala Outdated Show resolved Hide resolved
@cseed cseed force-pushed the emit-params branch 5 times, most recently from bbadd32 to a516570 Compare April 4, 2020 14:31
@cseed
Copy link
Collaborator Author

cseed commented Apr 4, 2020

A miracle. It finally passed. That was a real slog. I pushed a bunch of non-trivial changes, so it is probably good if you give a skeptical, fresh look. Summary of new changes:

  • added PType.literalPType that infers PTypes from Scala literals, use in a few places (emit for Literal, BroadcastRegionValue constructor from annotation, etc.)
  • require Table global and row types to be required
  • same for MatrixValue, but also cols and entries (the entries array, not individual entries, which an be missing)
  • Don't upcast globals in TableKeyBy and TableOrderBy
  • added EType setRequired
  • AbstractCodecSpecs assert row and global etypes are present at the toplevel, and setRequired(true) if they are coming from encoders written by previous versions
  • rename PType.copyFromType to PType.copyFromAdddres. Modify it so it can "downcast": convert to a PType with greater requiredness. This is used in converting TableValues to MatrixValues to satisfy the requiredness assertions.

Let me know if you have any questions.

@cseed cseed dismissed chrisvittal’s stale review April 4, 2020 18:30

addressed comments, passing

@chrisvittal
Copy link
Collaborator

I've been following along. So I've seen a lot of the new stuff already. I'll take another look.

Copy link
Collaborator

@chrisvittal chrisvittal left a comment

Choose a reason for hiding this comment

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

Let's see what shakes out.

@danking danking merged commit c877247 into hail-is:master Apr 4, 2020
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.

None yet

3 participants