Define sort! for AbstractDataFrame and fix issues of kwargs in sorting functions

[bkamins]

@nalimilan - do you see any risks in making sort! more flexible and allow SubDataFrame?
(I will add tests if we agree on the design)

I have also proposed to be more careful and error if columns alias but are not identical, but we might decide not to add this extra check.

[bkamins]

@nalimilan - I have added tests. It should be good for a review

In general what I propose is I think better. Already the previous method has quadratic complexity in the number of columns, but I think it is safer to error immediately when unsafe aliasing is used.

[nalimilan]

Looks good!

In general what I propose is I think better. Already the previous method has quadratic complexity in the number of columns, but I think it is safer to error immediately when unsafe aliasing is used.

I just wonder whether we should optimistically check and permute each column, and undo the changes if needed (as it should be super rare). But maybe that wouldn't make a difference for performance.

[bkamins]

But maybe that wouldn't make a difference for performance.

This is the point. It would only improve performance if we get an error which should be super rare.

[nalimilan]

This is the point. It would only improve performance if we get an error which should be super rare.

Wait, isn't that the contrary? It could be faster when no error happens, but it would be slower if it does because we would need to restore already permuted columns to their original state. But I agree the gain should be small anyway.

[bkamins]

undo the changes if needed

But how would you detect you need to undo this operation. The cost of doing this detection is quadratic in number of columns. So if we want to detect it we can do this detection immediately (and this is what I do now). Additionally we need to detect exact aliases and avoid permuting them (this was the behavior that we already had).

A different situation is e.g. in push! where indeed we can cheaply detect a problem post factum (as we can just check if column lengths are correct). However, doing sort! only shuffles values so we have no easy way to detect the problem.

[nalimilan]

I just meant we could have a single loop over columns with the contents of the two loops you have now, and in case an error happens we would roll back any already applied changes. But forget that, it's probably not worth it.

[bkamins]

I ended up having to standardize everything. Now all kwargs, following the rules we set in the 1.0 release have to be either scalars or vectors (tuples are not allowed - as it was announced we will not allow tuples). I have also improved docstrings, test coverage, and error checking.

[nalimilan]

Why not put this in function signatures instead? People should be used to the kind of MethodError that is printed. If we start checking the type of all arguments manually the codebase is going to get quite large. :-)

[bkamins]

Totally agreed. For some reason I started copying the old design. Now all kwargs have proper type restrictions.

[nalimilan]

This text (that we just added) needs to be adapted a bit depending on the function.

[bkamins]

right - fixed

[bkamins]

@nalimilan - let me know when you think it is OK and I will do a final check and merge (there is so much copy-paste in this PR that I want to double check everything before merging).

[bkamins]

Thank you!