From f02b974b198b4dd1cc3356b297418ea38899f8e4 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Sat, 5 Aug 2023 15:09:44 -0400 Subject: [PATCH 1/7] Fix instability in getproperty(::Schema, :types) --- src/Tables.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tables.jl b/src/Tables.jl index ddec790..3666978 100644 --- a/src/Tables.jl +++ b/src/Tables.jl @@ -500,7 +500,7 @@ function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {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)) + return types === nothing ? (T !== nothing ? T : nothing) : ntuple(i -> fieldtype(types, i), Val(fieldcount(types))) else throw(ArgumentError("unsupported property for Tables.Schema")) end From f3fa24a0cc8b3e1a767f15cfe253f4e2012d577e Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Sat, 5 Aug 2023 17:30:11 -0400 Subject: [PATCH 2/7] Add test for type stability of `.types` --- test/runtests.jl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/runtests.jl b/test/runtests.jl index 25d03c8..4054414 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -211,6 +211,12 @@ 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 _get_types(x) + end end @testset "Materializer" begin From 4d4c9621cdb38d856301a473218b5e3a5a8de974 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Sun, 6 Aug 2023 15:44:07 -0400 Subject: [PATCH 3/7] Avoid `ntuple` for `fieldcount` above 512 --- src/Tables.jl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Tables.jl b/src/Tables.jl index 3666978..5766754 100644 --- a/src/Tables.jl +++ b/src/Tables.jl @@ -500,7 +500,17 @@ function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {names return names === nothing ? getfield(sch, :storednames) : names elseif field === :types T = getfield(sch, :storedtypes) - return types === nothing ? (T !== nothing ? T : nothing) : ntuple(i -> fieldtype(types, i), Val(fieldcount(types))) + if types === nothing + return (T !== nothing ? T : nothing) + 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 From 08b48865244b328b63a00416617c7df3a8a0791e Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Sun, 6 Aug 2023 22:37:07 -0400 Subject: [PATCH 4/7] Add test to assert type instability for wide tables --- test/runtests.jl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/runtests.jl b/test/runtests.jl index 4054414..292f07e 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -214,8 +214,13 @@ end @testset "Type stability of schema(x).types" begin _get_types(X) = Tables.schema(X).types + x = (a=[1.0], b=[1.0]) - @inferred _get_types(x) + @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 From 9aa551a76601ec99fd4d4cec9294bde5e184d899 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Sun, 6 Aug 2023 22:38:57 -0400 Subject: [PATCH 5/7] Localize variable definition in getproperty --- src/Tables.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tables.jl b/src/Tables.jl index 5766754..1c6389b 100644 --- a/src/Tables.jl +++ b/src/Tables.jl @@ -499,8 +499,8 @@ 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) if types === nothing + T = getfield(sch, :storedtypes) return (T !== nothing ? T : nothing) else ncol = fieldcount(types) From edb7cc8d804eac7d97cd951fd98420116cd03323 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Thu, 19 Oct 2023 23:18:58 +0200 Subject: [PATCH 6/7] Update src/Tables.jl --- src/Tables.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tables.jl b/src/Tables.jl index 1c6389b..dfc0a5f 100644 --- a/src/Tables.jl +++ b/src/Tables.jl @@ -501,7 +501,7 @@ function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {names elseif field === :types if types === nothing T = getfield(sch, :storedtypes) - return (T !== nothing ? T : nothing) + return T !== nothing ? T : nothing else ncol = fieldcount(types) if ncol <= 512 From fc33a5a682a7fb15c25ebacbe5229c6d910650fc Mon Sep 17 00:00:00 2001 From: Miles Cranmer Date: Thu, 19 Oct 2023 23:07:55 +0100 Subject: [PATCH 7/7] Clean up `getproperty` redundancy --- src/Tables.jl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Tables.jl b/src/Tables.jl index dfc0a5f..e4fb110 100644 --- a/src/Tables.jl +++ b/src/Tables.jl @@ -500,8 +500,7 @@ function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {names return names === nothing ? getfield(sch, :storednames) : names elseif field === :types if types === nothing - T = getfield(sch, :storedtypes) - return T !== nothing ? T : nothing + return getfield(sch, :storedtypes) else ncol = fieldcount(types) if ncol <= 512