You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "baumgold (via GitHub)" <gi...@apache.org> on 2023/06/06 20:17:24 UTC
[GitHub] [arrow-julia] baumgold opened a new pull request, #456: implement default for SubArray
baumgold opened a new pull request, #456:
URL: https://github.com/apache/arrow-julia/pull/456
`similar(::Type{<:SubArray})` doesn't exist. This change should handle it gracefully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1581674295
I was able to reproduce this with the following:
```julia
julia> NT = @NamedTuple{x::Int, y::Union{Missing,Int}};
julia> data = NT[(x=1,y=2), (x=2,y=missing), (x=3,y=4), (x=4,y=5)];
julia> t = [(a=1,b=view(data,1:2)), (a=2,b=view(data,3:4)), missing];
julia> Arrow.toarrowvector(t);
ERROR: MethodError: no method matching SubArray{NamedTuple{(:x, :y), Tuple{Int64, Union{Missing, Int64}}}, 1, Vector{NamedTuple{(:x, :y), Tuple{Int64, Union{Missing, Int64}}}}, Tuple{UnitRange{Int64}}, true}(::UndefInitializer, ::Tuple{Int64})
```
Note after I added the suggested solution from this PR, I now get the error in #458:
```julia
julia> Arrow.ArrowTypes.default(::Type{SubArray{T,N,P,I,L}}) where {T,N,P,I,L} = Arrow.ArrowTypes.default(P);
julia> Arrow.toarrowvector(t);
ERROR: MethodError: Cannot `convert` an object of type Vector{NamedTuple{(:x, :y), Tuple{Int64, Union{Missing, Int64}}}} to an object of type SubArray{NamedTuple{(:x, :y), Tuple{Int64, Union{Missing, Int64}}}, 1, Vector{NamedTuple{(:x, :y), Tuple{Int64, Union{Missing, Int64}}}}, Tuple{UnitRange{Int64}}, true}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1579577077
One of the apps I maintain that relies on Arrow.jl for writing crashed when upgrading to Arrow v2.6 with the following error/stack:
```julia
MethodError: no method matching SubArray{NamedTuple{(:QuoteSetID, :OfferSize, :BidSize), Tuple{UInt16, Union{Missing, UInt32}, Union{Missing, UInt32}}}, 1, Arrow.Struct{NamedTuple{(:QuoteSetID, :OfferSize, :BidSize), Tuple{UInt16, Union{Missing, UInt32}, Union{Missing, UInt32}}}, Tuple{Arrow.Primitive{UInt16, Vector{UInt16}}, Arrow.Primitive{Union{Missing, UInt32}, Vector{UInt32}}, Arrow.Primitive{Union{Missing, UInt32}, Vector{UInt32}}}}, Tuple{UnitRange{Int64}}, true}(::UndefInitializer, ::Tuple{Int64})
Closest candidates are:
SubArray{T, N, P, I, L}(::Any, ::Any, ::Any, ::Any) where {T, N, P, I, L}
@ Base subarray.jl:19
Stacktrace:
[1] similar
@ ./abstractarray.jl:882
[2] similar
@ ./abstractarray.jl:880
[3] default
@ ~/.julia/packages/ArrowTypes/lFz2k/src/ArrowTypes.jl:346
[4] getindex
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/struct.jl:80 [inlined]
[5] iterate
@ ./abstractarray.jl:1220 [inlined]
[6] iterate
@ ./abstractarray.jl:1218 [inlined]
[7] #ToList#16
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/list.jl:113
[8] ToList
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/list.jl:100 [inlined]
[9] #arrowvector#18
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/list.jl:214
[10] arrowvector
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/list.jl:211 [inlined]
[11] #arrowvector#10
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:97
[12] #arrowvector#3
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:74
[13] #49
@ ./none:0
[14] iterate
@ ./generator.jl:47 [inlined]
[15] collect_to!
@ ./array.jl:840
[16] collect_to!
@ ./array.jl:848
--- the last 1 lines are repeated 1 more time ---
[18] collect_to_with_first!
@ ./array.jl:818
[19] collect
@ ./array.jl:792
[20] _totuple
@ ./tuple.jl:401 [inlined]
[21] Tuple
@ ./tuple.jl:369 [inlined]
[22] #arrowvector#48
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/struct.jl:93
[23] arrowvector
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/struct.jl:89 [inlined]
[24] #arrowvector#10
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:97
[25] #arrowvector#3
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:74
[26] arrowvector
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:58 [inlined]
[27] #toarrowvector#2
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:37
[28] toarrowvector
@ ~/.julia/packages/Arrow/86p6m/src/arraytypes/arraytypes.jl:34 [inlined]
[29] #145
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:329
[30] eachcolumn
@ ~/.julia/packages/Tables/AcRIE/src/utils.jl:70
[31] toarrowtable
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:326
[32] macro expansion
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:187 [inlined]
[33] macro expansion
@ ./task.jl:476 [inlined]
[34] write
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:177
[35] #122
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:58
[36] #open#409
@ ./io.jl:395
[37] open
@ ./io.jl:392 [inlined]
[38] #write#121
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:57 [inlined]
[39] write
@ ~/.julia/packages/Arrow/86p6m/src/write.jl:56 [inlined]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on a diff in pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on code in PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#discussion_r1220575400
##########
src/ArrowTypes/src/ArrowTypes.jl:
##########
@@ -348,6 +348,8 @@ function default(::Type{A}) where {A <: AbstractVector{T}} where {T}
return a
end
+default(::Type{A}) where {A <: SubArray} = default(Vector{eltype(A)})
Review Comment:
A better implementation might be:
```julia
default(::Type{SubArray{T,N,P,I,L}}) where {T,N,P,I,L} = default(P)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1579389448
Can you give a little more context around why we need this or what problem you ran into without?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1583042935
@quinnj - thanks for your feedback and suggestions. I've updated the code per your recommendations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1583202553
ArrowTypes tests pass but Arrow tests fail because they are not using the newly fixed ArrowTypes version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj merged pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj merged PR #456:
URL: https://github.com/apache/arrow-julia/pull/456
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1582584720
Oh wait, I think I understand now: you're saying _if_ we use your proposed change in this PR we end up with the error in #458. Ok, got it. Let me dig in a bit to think on a solution here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1581274157
@quinnj - any opinions/suggestions here? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on a diff in pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on code in PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#discussion_r1223052904
##########
src/ArrowTypes/src/ArrowTypes.jl:
##########
@@ -348,6 +348,8 @@ function default(::Type{A}) where {A <: AbstractVector{T}} where {T}
return a
end
+default(::Type{SubArray{T,N,P,I,L}}) where {T,N,P,I,L} = default(P)
Review Comment:
```suggestion
default(::Type{SubArray{T,N,P,I,L}}) where {T,N,P,I,L} = view(default(P), 0:-1)
```
The issue here is `default(P)` returns a `Vector`, but we want a SubArray, so my suggestion seems to solve it for me locally
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on a diff in pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on code in PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#discussion_r1223303727
##########
src/ArrowTypes/src/ArrowTypes.jl:
##########
@@ -348,6 +348,8 @@ function default(::Type{A}) where {A <: AbstractVector{T}} where {T}
return a
end
+default(::Type{SubArray{T,N,P,I,L}}) where {T,N,P,I,L} = default(P)
Review Comment:
Good suggestion, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1582581357
Ok, I better understand the issue _this_ PR is trying to fix and I agree there's an actual bug, but it seems different from what is reported in https://github.com/apache/arrow-julia/issues/458? @baumgold, can you confirm why you think it's the same issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1582620607
If we can commit my suggestion and then add @baumgold's repro as a test, I think this is good to go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1583911278
I commented out the test for now; will re-enable after we tag another release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] codecov-commenter commented on pull request #456: implement default for SubArray
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#issuecomment-1579413757
## [Codecov](https://app.codecov.io/gh/apache/arrow-julia/pull/456?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#456](https://app.codecov.io/gh/apache/arrow-julia/pull/456?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e427aef) into [main](https://app.codecov.io/gh/apache/arrow-julia/commit/5b6d412685bfebb7164694cd55339aa7fe974f74?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5b6d412) will **decrease** coverage by `0.03%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## main #456 +/- ##
==========================================
- Coverage 87.55% 87.53% -0.03%
==========================================
Files 26 26
Lines 3271 3272 +1
==========================================
Hits 2864 2864
- Misses 407 408 +1
```
| [Impacted Files](https://app.codecov.io/gh/apache/arrow-julia/pull/456?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [src/ArrowTypes/src/ArrowTypes.jl](https://app.codecov.io/gh/apache/arrow-julia/pull/456?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL0Fycm93VHlwZXMvc3JjL0Fycm93VHlwZXMuamw=) | `86.85% <0.00%> (-0.50%)` | :arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] baumgold commented on a diff in pull request #456: implement default for SubArray
Posted by "baumgold (via GitHub)" <gi...@apache.org>.
baumgold commented on code in PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#discussion_r1220575400
##########
src/ArrowTypes/src/ArrowTypes.jl:
##########
@@ -348,6 +348,8 @@ function default(::Type{A}) where {A <: AbstractVector{T}} where {T}
return a
end
+default(::Type{A}) where {A <: SubArray} = default(Vector{eltype(A)})
Review Comment:
A better implementation might be:
```julia
default(::Type{SubArray{T,N,P,I,L}) where {T,N,P,I,L} = default(P)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-julia] quinnj commented on a diff in pull request #456: implement default for SubArray
Posted by "quinnj (via GitHub)" <gi...@apache.org>.
quinnj commented on code in PR #456:
URL: https://github.com/apache/arrow-julia/pull/456#discussion_r1223638624
##########
test/runtests.jl:
##########
@@ -707,6 +707,15 @@ t = Arrow.Table(buf)
end
+@testset "# 456" begin
+
+NT = @NamedTuple{x::Int, y::Union{Missing,Int}}
+data = NT[(x=1,y=2), (x=2,y=missing), (x=3,y=4), (x=4,y=5)]
+t = [(a=1,b=view(data,1:2)), (a=2,b=view(data,3:4)), missing]
+@test Arrow.toarrowvector(t) isa Arrow.Struct
+
+end
Review Comment:
```suggestion
# @testset "# 456" begin
# NT = @NamedTuple{x::Int, y::Union{Missing,Int}}
# data = NT[(x=1,y=2), (x=2,y=missing), (x=3,y=4), (x=4,y=5)]
# t = [(a=1,b=view(data,1:2)), (a=2,b=view(data,3:4)), missing]
# @test Arrow.toarrowvector(t) isa Arrow.Struct
# end
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org