-
Notifications
You must be signed in to change notification settings - Fork 4
Adding first
, last
and describe
convenience functions.
#42
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
base: main
Are you sure you want to change the base?
Conversation
@jpsamaroo how does this implementation of |
src/table/dtable.jl
Outdated
return table | ||
end | ||
|
||
chunk_length = chunk_lengths(table)[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chunk lengths are not guarenteed to be equal
some may even be empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @krynju. If this is the case, is there any way to retrieve the original chunksize
? If I'm not wrong it's not stored as a property of DTable
s.
On another note: suppose for a DTable
I have chunksize
greater than number of rows in the table. In that case, won't I lose information about what chunksize
I passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it was an early design decision to make chunksize an argument of the constructor for the initial partitioning and later ignore it (and for that reason not store it either)
I think including the original chunksize in the logic would also be a bit confusing and would make it more complex, but if we have any use case for that then we can revisit this
I did think of caching the current chunk sizes, because generally that information doesn't change in a dtable (after you manipulate a dtable it becomes a new dtable)
We already cache the schema so a similar mechanism could be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and for this you can just use chunk_legths as you did
https://github.com/JuliaParallel/DTables.jl/blob/9fcbe237e0c6ddd6b6f2880f33347efe99a76fdd/src/table/dtable.jl#L252C10-L255
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On another note: suppose for a DTable I have chunksize greater than number of rows in the table. In that case, won't I lose information about what chunksize I passed?
You will and you will only get one partition in the dtable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it was an early design decision to make chunksize an argument of the constructor for the initial partitioning and later ignore it (and for that reason not store it either)
I think including the original chunksize in the logic would also be a bit confusing and would make it more complex, but if we have any use case for that then we can revisit this
I did think of caching the current chunk sizes, because generally that information doesn't change in a dtable (after you manipulate a dtable it becomes a new dtable) We already cache the schema so a similar mechanism could be used
@krynju how about this: to get the chunksize, can I get the maximum value from chunk_lengths? Certainly this maximum should be the original chunksize
, except for a boundary case where chunksize
is greater than the number of rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not guaranteed. Why do you need the original chunksize?
src/table/dtable.jl
Outdated
extra_chunk_rows = rowtable(fetch(extra_chunk)) | ||
new_chunk = Dagger.tochunk(sink(extra_chunk_rows[1:needed_rows])) | ||
required_chunks = vcat(table.chunks[1:num_full_chunks], [new_chunk]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to do this with with Dagger.@spawn
and make the last dtable chunk just a thunk (so result of Dagger.@spawn
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krynju how does it look now? I've used the maximum among all chunk_lengths
to get the original chunk size, and made the last chunk a thunk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the actual chunk lengths and not a maximum of them
When you call first(d,50) it should go something like this
(not valid code, just writing the idea)
s = 50
csum=0
chunks = []
for (cl,chunk) in zip(chunk_lengths(d), d.chunks)
if csum + cl > s
# do the thing with spawn, this is the last one and we need to make a thunk from it and cut it
push!(chunks, the_cut_thunk)
else
csum += cl
push!(chunks, chunk)
end
return DTable(chunks)
end
Still a draft PR.
This PR adds implementations of the
first
,last
anddescribe
convenience functions.Examples
Here is how
first
works:TODO:
first
.last
.describe
.