search

JuliaData / DataTables.jl

(DEPRECATED) A rewrite of DataFrames.jl based on Nullable
Other
29 stars 11 forks source link

Consolidate DataTable constructors and remove autopromotion #53

Closed cjprybol closed 7 years ago

cjprybol commented 7 years ago

Consolidating the constructors minimized the number of places where auto promotion could take place. The new constructor recycles scalars such that if DataTable is created with a mix of scalars and vectors the scalars will be recycled to the same length as the vectors. Fixes an outstanding bug where scalar recycling only worked if the scalar assignments came after the vector assignments of the desired length, see: https://github.com/JuliaStats/DataFrames.jl/pull/882. Tests that used to assume NullableArray promotion now explicitly use NullableArrays and new constructor tests have been added to test changes.

This is dependent on https://github.com/JuliaStats/NullableArrays.jl/pull/189 as I've re-used the _isnullable function added in that PR for more robust nullability checking.

nalimilan commented 7 years ago

For such a simple function, I'd rather redefine _isnullable here rather than importing it from NullableArrays. Relying on non-exported functions from other packages should be avoided where possible.

EDIT: I had confused this _isnullable method with _isnull. Anyway, turns out we don't need it at all here, see review.

quinnj commented 7 years ago

Closing in favor of #64; sorry for the PR gymnastics, but GitHub won't let me push to this branch, so I had to create a new one (preserving @cjprybol 's original commit here)