Bug in merge for NamedTuple

[bkamins]

Here is a MWE:

julia> x = Iterators.reverse(pairs((a=1,b=2)))
pairs(::NamedTuple) with 2 entries:
  :b => 2
  :a => 1

julia> merge(NamedTuple(), x)
(a = 1, b = 2)

The problem is that merge uses only :data field, but ignores the order specified by the :itr field.

[JeffBezanson]

Huh --- I'm not sure the use of Pairs in this reverse method is valid. It's possible to construct one with any keys you want, but I don't think the type is intended to provide general "views" of collections.

[bkamins]

For a reference: it is not NamedTuple specific:

julia> dump(Iterators.reverse(pairs((a=11,b=22,c=33,d=44))))
Base.Iterators.Pairs{Symbol, Int64, Base.Iterators.Reverse{NTuple{4, Symbol}}, NamedTuple{(:a, :b, :c, :d), NTuple{4, Int64}}}
  data: NamedTuple{(:a, :b, :c, :d), NTuple{4, Int64}}
    a: Int64 11
    b: Int64 22
    c: Int64 33
    d: Int64 44
  itr: Base.Iterators.Reverse{NTuple{4, Symbol}}
    itr: NTuple{4, Symbol}
      1: Symbol a
      2: Symbol b
      3: Symbol c
      4: Symbol d

julia> dump(Iterators.reverse(pairs((11,22,33,44))))
Base.Iterators.Pairs{Int64, Int64, StepRange{Int64, Int64}, NTuple{4, Int64}}
  data: NTuple{4, Int64}
    1: Int64 11
    2: Int64 22
    3: Int64 33
    4: Int64 44
  itr: StepRange{Int64, Int64}
    start: Int64 4
    step: Int64 -1
    stop: Int64 1

julia> dump(Iterators.reverse(pairs([11,22,33,44])))
Base.Iterators.Pairs{Int64, Int64, Base.Iterators.Reverse{LinearIndices{1, Tuple{Base.OneTo{Int64}}}}, Vector{Int64}}
  data: Array{Int64}((4,)) [11, 22, 33, 44]
  itr: Base.Iterators.Reverse{LinearIndices{1, Tuple{Base.OneTo{Int64}}}}
    itr: LinearIndices{1, Tuple{Base.OneTo{Int64}}}
      indices: Tuple{Base.OneTo{Int64}}
        1: Base.OneTo{Int64}
          stop: Int64 4

[JeffBezanson]

Right; I'm wondering for example if that method should reverse the collection too, or be replaced with several more specific methods, or something else.

[JeffBezanson]

Aha:

values(v::Pairs) = getfield(v, :data) # TODO: this should be a view of data subset by itr

So Pairs is enough of a view to write a comment about it, but not enough of a view to actually work that way :)

One thing at stake here is that if Pairs can do arbitrary re-orderings, then merge(::NamedTuple, ::Pairs{NamedTuple}) cannot be optimized.

[bkamins]

I would prioritize correctness over speed (as clearly what I report is a bug). I think we could allow :itr to be nothing, in which case the order of :data could be taken (as this is probably 99% of cases).

I have not checked if it would break anything but it seems it should not.

[JeffBezanson]

Idea: instead of implementing reverse(::Pairs) = Pairs(...), return a Reverse iterator for that, and implement iterate for Reverse{<:Pairs}.