[compiler] Add If and MakeStruct Simplifications
#14231
Conversation
MakeStruct("a" -> GetField(o, "x"), ...) -> CastRename(SelectFields(o, ["x"..]), newtype)
If(IsNA(x), NA(x.typ), x) -> x
ehigham
dismissed
patrick-schultz’s stale review
submission changed owing to test failures
ehigham
dismissed
patrick-schultz’s stale review
Possibly dangerous change to SSubsetStruct.equals
|
Merging in #14233 causes the failure in |
hail/src/main/scala/is/hail/types/physical/stypes/concrete/SSubsetStruct.scala
Outdated
Show resolved
Hide resolved
| new SSubsetStruct(newParent.asInstanceOf[SBaseStruct], newVirtualType.fieldNames) { | ||
| override lazy val newToOldFieldMapping: Map[Int, Int] = | ||
| SSubsetStruct.this.newToOldFieldMapping | ||
| override lazy val virtualType: TStruct = | ||
| newVirtualType | ||
| } |
There was a problem hiding this comment.
I don't like the anonymous subclassing here. It makes it difficult to say what an SSubsetStruct really is. I think the core issue forcing the need subclass is that newToOldFieldMapping should really be a constructor argument (as also evidenced by its appearence in the definition of equality). Then there should be no need to subclass.
Concretely, I suggest making the default constructor
class SSubsetStruct(
private val parent: SBaseStruct,
private val fieldNames: IndexedSeq[String],
private val oldToNewFieldMapping
) extends SBaseStruct { ...
and adding a convenience constructor taking only the first two, and computing a default oldToNewFieldMapping as we do now.
There was a problem hiding this comment.
Sure - that sounds like a better change.
Doesn't seem to be an issue anymore... |
| override lazy val fieldTypes: IndexedSeq[SType] = | ||
| Array.tabulate(size) { i => | ||
| parent | ||
| .fieldTypes(newToOldFieldMapping(i)) | ||
| .castRename(rename.fields(i).typ) | ||
| } |
There was a problem hiding this comment.
It looks like this assumes that the fields of restrict have the same virtual type as the corresponding fields in parent. I'm in favor of not making SStructView support deep subsetting for now to keep things simple, but in that case, having an entire restrict type is redundant and, I think, confusing. It would be sufficient to just use a list of parent fields to subset to (but then still the full rename struct type).
Add the following transformations:
The changes to
SStructView(neeSSubsetStruct) were as a result of a bud that prevented subsetting and then renaming to an excluded field, ie{x, y, z} subset {z} rename {x}. NowSStructViewleaves its parentSTypeunmodified and casts loads through the parent to the appropriateSValue.