ENH,BUG: Bind data directly for row group ranges #122
Conversation
This avoids one unnecssary copy, but also avoids `get_physical_array()` in general, which appears to be a pattern that is not ideal typically (and specifically with streaming). Signed-off-by: Sebastian Berg <[email protected]>
seberg
force-pushed
the
parquet-bind-rowgroups
branch
from
db5ea0b to
569cc47
Compare
|
@reazulhoque did this work out? Would be nice to confirm it's right, although, so long we trust the trick with the shared pointer keep alive in the closure, I think this is an improvement either way. |
| .write_accessor<legate::Rect<1>, 1, false>() | ||
| .ptr(0); | ||
| std::copy(row_group_ranges.begin(), row_group_ranges.end(), ptr); | ||
| auto alloc = legate::ExternalAllocation::create_sysmem( |
There was a problem hiding this comment.
This is a bit weird. Normally wouldn't you write to a store inside the task? I think its even ok now to write to a broadcast store.
Also if I did row_group_ranges->push_back() in a later part of the code, the vector could resize and delete the allocation used in ExteranAllocation.
There was a problem hiding this comment.
Good, point, will at least try and make it const.
Normally wouldn't you write to a store inside the task
Yeah, I think this should be refactored to a small task (ideally one worker per file maybe). This is more of a hot-fix to help the streaming, and for those tasks, it would be nice to use unbound results, but that would just break the streaming again right now...
|
@seberg unfortunately it seems like to make this work we'll need to force the submit of previous tasks as well. |
|
Superseded by other PR. |
This avoids one unnecssary copy, but also avoids
get_physical_array()in general, which appears to be a pattern that is not ideal typically (and specifically with streaming).