add firstindex check and fix a bug in mapcols!

[bkamins]

We currently assume that vectors are 1-based in DataFrames.jl. This PR makes sure we check for that. Not checking this will lead to bugs. Also I found some small bug in mapcols! in the process related to AbstractRange handling.

I think we should decide on this before 1.0 release (it is better to be strict I think now; we can be more flexible later).

I will add tests, NEWS.md and update documentation with this PR after we agree that we want to add these changes.

[bkamins]

@nalimilan - are you OK with these proposed changes. If yes I would finalize the PR

[bkamins]

@nalimilan - the issue came up again in #2604. Are you OK to finalize this PR in the form I have proposed?

[bkamins]

@nalimilan - this should be good to have a look at (especially corner cases in the tests).

[bkamins]

I had to split the tests into a separate file as OffsetArrays.jl does not run on Julia 1.0 (due to its dependency).

[bkamins]

CI and coverage passes cleanly now.

[nalimilan]

What was the problem with this syntax? Compilation overhead?

[bkamins]

yes - I try to remove broadcasting in the internal code to reduce latency a bit (also I thought that the syntax might seem too magic for casual reader).

[bkamins]

The difference is:

current release:

julia> using DataFrames

julia> df = DataFrame(a=1)
1×1 DataFrame
 Row │ a     
     │ Int64 
─────┼───────
   1 │     1

julia> @time mapcols!(length, df)
  0.003640 seconds (2.75 k allocations: 146.932 KiB)
1×1 DataFrame
 Row │ a     
     │ Int64 
─────┼───────
   1 │     1

this PR:

julia> using DataFrames

julia> df = DataFrame(a=1)
1×1 DataFrame
 Row │ a     
     │ Int64 
─────┼───────
   1 │     1

julia> @time mapcols!(length, df)
  0.000015 seconds (4 allocations: 256 bytes)
1×1 DataFrame
 Row │ a     
     │ Int64 
─────┼───────
   1 │     1

so it is not a big deal (but we have to iterate vs anyway to check firstindex).

[nalimilan]

Wow. I wouldn't have expected such a large difference in the number of allocations.

[bkamins]

broadcasting is heavy (when doing the PRs that Tim Holy prompted I will review the whole codebase against using broadcasting). Of course here the difference is one-time and not that big.

[yurivish]

@bkamins I just learned about the function Base.require_one_based_indexing – are you aware of it? It's mentioned in the devdocs here and I'm not sure if it's sanctioned for use in user code, but the devdocs are written as if it is.

It is defined here: https://github.com/JuliaLang/julia/blob/ae53238c45a0cd6dafc6e121f4daaa93143bf627/base/abstractarray.jl#L103

[bkamins]

@yurivish - thank you for looking into this. I preferred custom checks for the following reasons (although the function seems to be a part of "official" API even if it is not exported):

• it is not available in Julia 1.0 which we support
• we can throw custom error message if we handle the check ourselves
• it is, easy to make this check

[nalimilan]

Mmm, is the CI failure related to recent changes in Tables?

[bkamins]

Yes, I will rebase and merge after CI passes.

[bkamins]

Thank you!