Repository: JuliaData/Tables.jl (julia)

Issue:
Type stability of `Tables.matrix(::NamedTuple)`
I'm descending some type instability issues in my package and realized that `Tables.matrix(::NamedTuple)` is not type stable. Would this be feasible to implement?

For example:

```julia
julia> using Tables

julia> X = (x=randn(32), y=randn(32));

julia> typeof(Tables.matrix(X))
Matrix{Float64} (alias for Array{Float64, 2})

julia> @code_warntype Tables.matrix(X)
MethodInstance for Tables.matrix(::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}})
  from matrix(table; transpose) @ Tables ~/.julia/packages/Tables/AcRIE/src/matrix.jl:84
Arguments
  #self#::Core.Const(Tables.matrix)
  table::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}
Body::Matrix
1 ─ %1 = Tables.:(var"#matrix#85")(false, #self#, table)::Matrix
└──      return %1
```

So the type inference is not able to infer that this is a `Matrix{Float64}`, only that it is a `Matrix{Any}`, despite the method call being specialized to a NamedTuple with `Vector{Float64}` for columns. 

Version info:

```
Julia Version 1.10.0-beta1
Commit 6616549950e (2023-07-25 17:43 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 8 × Apple M1 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
  Threads: 8 on 6 virtual cores
Environment:
  JULIA_FORMATTER_SO = /Users/mcranmer/julia_formatter.so
  JULIA_NUM_THREADS = auto
  JULIA_EDITOR = code
```

Pull request description:
Fixes #339. The issue was that inside `getproperty`, the generator for table types:

```julia
Tuple(fieldtype(types, i) for i = 1:fieldcount(types))
```

is type unstable.

This PR changes it to:

```julia
ntuple(i -> fieldtype(types, i), Val(fieldcount(types)))
```

which is stable.

**Edit:** 4d4c9621cdb38d856301a473218b5e3a5a8de974 changes it so that `getproperty` falls back to the type unstable `Tuple` when the number of columns goes above 512. This prevents any major slow downs in compile time.

This gives some major speed ups in both runtime and compile time:

![plot](https://github.com/JuliaData/Tables.jl/assets/7593028/57266bd9-ecb9-4dea-b852-d1de72d97d73)

e.g., here is an inspection of `@code_warntype` on a function that returns `.types` for a schema:

```julia
julia> X = (x=randn(32), y=randn(32));

julia> f(X) = Tables.schema(X).types
f (generic function with 1 method)

julia> @code_warntype f(X)
MethodInstance for f(::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}})
  from f(X) @ Main REPL[5]:1
Arguments
  #self#::Core.Const(f)
  X::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}
Body::Tuple{DataType, DataType}
1 ─ %1 = Tables.schema::Core.Const(Tables.schema)
│   %2 = (%1)(X)::Core.Const(Tables.Schema:
 :x  Float64
 :y  Float64)
│   %3 = Base.getproperty(%2, :types)::Core.Const((Float64, Float64))
└──      return %3
```

whereas before this change, it was inferring this as `Vararg{DataType}` (bad).

@quinnj @bkamins could you please review and merge when you get a chance? This is necessary to fix a variety of type instabilities in MLJ interfaces.

cc @ablaom @OkonSamuel. This should speed up calls to `fit` and `predict` across the MLJ ecosystem as it removes a key type instability.

Thanks!
Miles

Patch:
diff --git a/src/Tables.jl b/src/Tables.jl
index ddec790..e4fb110 100644
--- a/src/Tables.jl
+++ b/src/Tables.jl
@@ -499,8 +499,17 @@ function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {names
     if field === :names
         return names === nothing ? getfield(sch, :storednames) : names
     elseif field === :types
-        T = getfield(sch, :storedtypes)
-        return types === nothing ? (T !== nothing ? T : nothing) : Tuple(fieldtype(types, i) for i = 1:fieldcount(types))
+        if types === nothing
+            return getfield(sch, :storedtypes)
+        else
+            ncol = fieldcount(types)
+            if ncol <= 512
+                # Type stable, but slower to compile
+                return ntuple(i -> fieldtype(types, i), Val(ncol))
+            else
+                return Tuple(fieldtype(types, i) for i=1:ncol)
+            end
+        end
     else
         throw(ArgumentError("unsupported property for Tables.Schema"))
     end


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 25d03c8..292f07e 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -211,6 +211,17 @@ end
 
     # 228
     @test Tables.columntable(NamedTuple[]) === NamedTuple()
+
+    @testset "Type stability of schema(x).types" begin
+        _get_types(X) = Tables.schema(X).types
+
+        x = (a=[1.0], b=[1.0])
+        @inferred Tuple{Type{Float64},Type{Float64}} _get_types(x)
+
+        # Trigger other branch which lacks type stability:
+        x_wide = NamedTuple([Symbol("x$(i)") => [1.0] for i=1:513])
+        @inferred NTuple{513,DataType} _get_types(x_wide)
+    end
 end
 
 @testset "Materializer" begin