[hail] LoadPlink ptype #6421
[hail] LoadPlink ptype #6421
Conversation
| @@ -505,6 +505,8 @@ class RegionValueBuilder(var region: Region) { | |||
| addRegionValue(t, uis.region, uis.aoff) | |||
| } | |||
|
|
|||
| def addAnnotation(t: PType, a: Annotation): Unit = addAnnotation(t.virtualType, a) | |||
There was a problem hiding this comment.
To avoid changing the 70 invocations of addAnnotation
There was a problem hiding this comment.
This is the wrong signature -- we should either have it be the virtual type, or no type.
| @@ -12,7 +13,9 @@ class TableTypeSerializer extends CustomSerializer[TableType](format => ( | |||
| { case tt: TableType => JString(tt.toString) })) | |||
|
|
|||
| case class TableType(rowType: TStruct, key: IndexedSeq[String], globalType: TStruct) extends BaseType { | |||
| lazy val canonicalRVDType = RVDType(rowType.physicalType, key) | |||
| lazy val canonicalPType = PType.canonical(rowType).asInstanceOf[PStruct] | |||
There was a problem hiding this comment.
lazy since derived from constructor arg, which is immutable
| @@ -3,7 +3,7 @@ package is.hail.methods | |||
| import is.hail.HailContext | |||
| import is.hail.annotations._ | |||
| import is.hail.expr.ir._ | |||
| import is.hail.expr.types.physical.PString | |||
| import is.hail.expr.types.physical.{PFloat64, PInt64, PString, PStruct} | |||
There was a problem hiding this comment.
Obviously all #6416
There was a problem hiding this comment.
Again, not sure why merge didn't modify the diff: https://github.com/hail-is/hail/pull/6416/files#diff-974b03f89566a9d87c87b58f59987942R6
|
|
||
| val signature = TStruct(("id", TString()), ("fam_id", TString()), ("pat_id", TString()), | ||
| ("mat_id", TString()), ("is_female", TBoolean()), phenoSig) | ||
| val signature = PStruct(("id", PString()), ("fam_id", PString()), ("pat_id", PString()), |
There was a problem hiding this comment.
Not certain this change is worthwhile, because it's only used in Table, and immediately converted back to virtualType.
There was a problem hiding this comment.
This violates the no-ptypes-if-no-offheap-data principle, but I think this is probably right -- we'll want to make some of these fields required, when we start doing that everywhere, and we won't be able to do that on virtual types.
| @@ -148,6 +149,31 @@ object Table { | |||
| ).keyBy(key, isSorted) | |||
| } | |||
|
|
|||
| def apply( | |||
There was a problem hiding this comment.
Not sure why this is still appearing as a diff:
https://github.com/hail-is/hail/pull/6416/files#diff-fc543ff0a72f412767e8393c89ae41eeR152
|
mind rebasing and removing the stacked label? will review then |
|
Done |
|
|
||
| val signature = TStruct(("id", TString()), ("fam_id", TString()), ("pat_id", TString()), | ||
| ("mat_id", TString()), ("is_female", TBoolean()), phenoSig) | ||
| val signature = PStruct(("id", PString()), ("fam_id", PString()), ("pat_id", PString()), |
There was a problem hiding this comment.
This violates the no-ptypes-if-no-offheap-data principle, but I think this is probably right -- we'll want to make some of these fields required, when we start doing that everywhere, and we won't be able to do that on virtual types.
Stacked on #6416