-
Notifications
You must be signed in to change notification settings - Fork 25
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.getindex
over allocating
#104
Comments
Pinging @quinnj and @andyferris for visibility. |
Thanks for noticing this and the contribution @m-wells |
@m-wells LazyTables seems really cool - some great ideas there. I've been wanting a "lazy" row for a long time but never got around to it. To be honest I haven't had the bandwidth to give many of the packages I started the love they deserve. If you really want "All the good of TypedTables.jl but FASTER and without as many allocations!" that sounds really great to me - I'd probably use it myself! :) All that is to say, if you want contribute to and help maintain TypedTables you would be very welcome to work here (I don't have much attachment to the existing code and hopefully wouldn't get in your way, and a breaking change to the |
I understand that 100%.
Sure.
If you are willing to drop NamedTuple as the eltype then most of the wrapper code in TypedTables.jl/src/TypedTables.jl Lines 16 to 43 in 0b86a6a
The definition of
I have other ideas and none of them involve changing how users interface with Table, just backend stuff. |
True that. And yes, I am willing. The only thing I might add is it would be nice if there were some kind of row type shared between different different tables (like the other tables here like the dictionary ones, but also it would be good to at least theoretically be able to support other Tables.jl tables or even DataFrames |
In the meantime @m-wells I have added you as a maintainer. Welcome, and thank you! :) Please free to merge #103. You are also welcome to make releases (following semver) - let me know if you aren't sure how that works. As a general development process it would be nice to follow a convention of having pull requests reviewed/approved, but if people are not available for please continue to make progress rather than getting stuck (use your judegement how long is too long). |
I don't see a particular use case for an struct TypedRow{K, V} <: Tables.AbstractRow
data::NamedTuple{K, V}
row::Int
end
struct Table{K, N, V} <: AbstractArray{TypedRow{K, V}, N}
data::NamedTuple{K, V}
# inner constructor to check equal length columns perhaps
end
struct FlexRow{A<:AbstractArray} <: Tables.AbstractRow
table::A
row::Int
end I haven't played with |
I'm tacking onto this issue since it's related, but please let me know if you'd prefer that I open a new issue. I noticed that CI fails for julia 1.0.5 with @m-wells's awesome updates to I locally modified the @inline function Base.getindex(t::Table{T}, i::Int) where {T}
@boundscheck checkbounds(t, i)
old = convert(T, map(col -> @inbounds(getindex(col, i)), columns(t)))::T
new = T(@inbounds(getindex(col, i)) for col in columns(t))
@assert old === new
return new
end
@inline function Base.getindex(t::Table{T}, i::Int...) where {T}
@boundscheck checkbounds(t, i...)
old = convert(T, map(col -> @inbounds(getindex(col, i...)), columns(t)))::T
new = T(@inbounds(getindex(col, i...) for col in columns(t)))
@assert old === new
return new
end Technically, for the failing test, only the first one matters, but I did both just for completeness. Both @Select and @Compute on Tables: Error During Test at /home/anirudh/projects/TypedTables.jl/test/Table.jl:169
Test threw exception
Expression: #= /home/anirudh/projects/TypedTables.jl/test/Table.jl:169 =# @inferred(mapreduce(s, ((acc, row)->begin
acc + row.sum
end), t; init=0.0)) === 18.0
return type Float64 does not match inferred return type Any
Stacktrace:
[1] error(::String) at ./error.jl:33
[2] macro expansion at /home/anirudh/projects/TypedTables.jl/test/Table.jl:169 [inlined]
[3] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
[4] macro expansion at /home/anirudh/projects/TypedTables.jl/test/Table.jl:151 [inlined]
[5] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
[6] top-level scope at /home/anirudh/projects/TypedTables.jl/test/Table.jl:2 Honestly, I'm a bit stuck here. My understanding of |
I'm still not sure why the changed submitted by @m-wells broke that test of Julia 1.0.5, but adding T(@inbounds(getindex(col, i)) for col in columns(t))::T On Julia 1.0.5, this does add an allocation for some reason, but it doesn't add an allocation on Julia 1.8 (still just 1 allocation total as shown in the earlier example by @m-wells). If you think this is an acceptable solution, I can open a PR. I'd still be curious if someone has an explanation for what was wrong before though. |
Any update on the overall proposal to combine work with LazyTables? I note that TypedTables has many more recent commits. What is entailed in "merging" the code to get LazyRows and other benefits with the cost of some breaking changes that Andy seems willing to accept. How can I help? |
I have a pull request that addresses this issue.
The text was updated successfully, but these errors were encountered: