Skip to content
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

Changes to append! inconpatible with TypedTables.jl #49748

Closed
MasonProtter opened this issue May 11, 2023 · 1 comment · Fixed by #49754
Closed

Changes to append! inconpatible with TypedTables.jl #49748

MasonProtter opened this issue May 11, 2023 · 1 comment · Fixed by #49754

Comments

@MasonProtter
Copy link
Contributor

MasonProtter commented May 11, 2023

It seems that this commit a20a3d0 (cc @aviatesk) makes append! no longer work with TypedTables.jl

julia> VERSION
v"1.10.0-DEV.1247"

julia> using TypedTables

julia> tints = Table(a = [1], b = [2])
Table with 2 columns and 1 row:
     a  b
   ┌─────
 11  2

julia> supertype(typeof(tints))
AbstractVector{@NamedTuple{a::Int64, b::Int64}} (alias for AbstractArray{@NamedTuple{a::Int64, b::Int64}, 1})

julia> append!(tints, [(a=3, b=4)])
ERROR: MethodError: no method matching __inbounds_setindex!(::Table{@NamedTuple{a::Int64, b::Int64}, 1, @NamedTuple{a::Vector{Int64}, b::Vector{Int64}}}, ::@NamedTuple{a::Int64, b::Int64}, ::Int64)

Closest candidates are:
  __inbounds_setindex!(::Array{T}, ::Any, ::Int64) where T
   @ Base array.jl:1019
  __inbounds_setindex!(::Array{T}, ::Any, ::Int64, ::Int64, ::Int64...) where T
   @ Base array.jl:1021

Stacktrace:
 [1] _append!(a::Table{@NamedTuple{a::Int64, b::Int64}, 1, @NamedTuple{a::Vector{Int64}, b::Vector{Int64}}}, ::Base.HasShape{1}, iter::Vector{@NamedTuple{a::Int64, b::Int64}})
   @ Base ./array.jl:1189
 [2] append!(a::Table{@NamedTuple{a::Int64, b::Int64}, 1, @NamedTuple{a::Vector{Int64}, b::Vector{Int64}}}, iter::Vector{@NamedTuple{a::Int64, b::Int64}})
   @ Base ./array.jl:1178
 [3] top-level scope
   @ REPL[24]:1

whereas on earlier versions that worked:

julia> append!(tints, [(a=3, b=4)])
Table with 2 columns and 2 rows:
     a  b
   ┌─────
 11  2
 23  4

julia> VERSION
v"1.9.0-rc3"
aviatesk added a commit that referenced this issue May 11, 2023
#47154 mistakenly added `@_safeindex` macro on the
`_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)` method,
although `@_safeindex` is only valid for builtin vectors i.e. `Vector`.

This commit adds `isa` check so that `@_safeindex` is only applied to
builtin vectors. The `isa` check should be removed at compile time, so
it should not affect the runtime performance.

closes #49748
@aviatesk
Copy link
Member

Thanks for your report. Submitted a fix at #49754.

aviatesk added a commit that referenced this issue May 12, 2023
#47154 mistakenly added `@_safeindex` macro on the
`_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)` method,
although `@_safeindex` is only valid for builtin vectors i.e. `Vector`.

This commit adds `isa` check so that `@_safeindex` is only applied to
builtin vectors. The `isa` check should be removed at compile time, so
it should not affect the runtime performance.

closes #49748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants