[compiler] don't force TableRead to produce a TableValue #13229
Conversation
patrick-schultz
force-pushed
the
table-reader-cleanup
branch
from
424b5f3
to
ce9ec06
Compare
patrick-schultz
changed the title
|
Put WIP back on, noticed something I still want to look into. |
|
Alright, should be ready now. |
| final def lower(ctx: ExecuteContext, requestedType: TableType, dropRows: Boolean): TableStage = | ||
| if (dropRows) { | ||
| val globals = lowerGlobals(ctx, requestedType.globalType) | ||
|
|
||
| TableStage( | ||
| globals, | ||
| RVDPartitioner.empty(ctx, requestedType.keyType), | ||
| TableStageDependency.none, | ||
| MakeStream(FastIndexedSeq(), TStream(TStruct.empty)), | ||
| (_: Ref) => MakeStream(FastIndexedSeq(), TStream(requestedType.rowType))) | ||
| } else | ||
| _lower(ctx, requestedType) | ||
|
|
||
| def _lower(ctx: ExecuteContext, requestedType: TableType): TableStage |
There was a problem hiding this comment.
I'm not sure what this change adds other than a template pattern which I generally avoid whenever possible. I preferred keeping this logic in LowerTableIr.
How is _lower different to lower? Who else uses lower with dropRows other than TableRead? _lower seems like something that shouldn't be public and calling it directly seems like a mistake.
There was a problem hiding this comment.
This is a pattern we use occasionally. _lower should really be protected, I'll make that change, and it is only called directly in lower, which is final. So _lower is the customization point for subclasses.
There was a problem hiding this comment.
What I was trying to understand is why do we need _lower at all when TableRead seems to be the only place that uses dropRows?
There was a problem hiding this comment.
Ah, I understand now. That's fair. Addressed.
| def toExecuteIntermediate(ctx: ExecuteContext, requestedType: TableType, dropRows: Boolean): TableExecuteIntermediate = { | ||
| assert(!dropRows) | ||
| TableExecuteIntermediate(_lower(ctx, requestedType)) |
There was a problem hiding this comment.
I like this interface change. One thing I'd suggest is to remove the TableExecuteIntermediate companion object and construct TableStageIntermediate and TableValueIntermediate directly.
There was a problem hiding this comment.
That's not a bad change, but given that it will all be going away when everything is lowered, I don't think there's much benefit.
patrick-schultz
force-pushed
the
table-reader-cleanup
branch
from
a702b5b
to
3b817ba
Compare
- this is a great change
#13313) On the current release (0.2.119), `is.hail.rvd.RVD.localSort` attempts to serialize an entire `is.hail.rvd.RVD` object when called, which causes some operations, like reading a set of VCF files, to fail. That particular operation no longer fails due to some combination of the changes made to reading VCF files in #13206 and #13229, but the serialization could still cause issues if `is.hail.rvd.RVD.localSort` is called in another context. See https://hail.zulipchat.com/#narrow/stream/123011-Hail-Query-Dev/topic/Possible.20bug.20in.20Hail.200.2E2.2E119/near/378610151 for more information.
Currently
TableRead.executealways produces aTableValueIntermediate, even though almost allTableReaders are lowerable, so could produce aTableStageIntermediate.This pr refactors
TableReaderto allow producing aTableStageIntermediatein most cases, and to make it clearer which readers still need to be lowered (onlyTableFromBlockMatrixNativeReader,MatrixVCFReader, andMatrixPLINKReader). It also deletes some now dead code.