sort: implement -k and -t support

[miDeb]

For #1728.

This allows to specify keys after the -k flag and a custom field
separator using -t.

Not sure if I've missed something... For reference GNU coreutils documentation about sort , POSIX spec.

[miDeb]

There's a test failure (of a test I added) saying thread 'test_sort::test_keys_invalid_field_zero' panicked at 'Broken pipe (os error 32)', tests/common/util.rs:733:37. Is this a problem with the test or something else?

[miDeb]

I think I want to implement the missing functionality first, before requesting a review

[kimono-koans]

Just FYI, I've been working on sort too. See most recently #2008. Pleased to let you implement -k and -t, but be aware there is some functionality that I've added and I'd hate for us to needlessly conflict. If you need my help with anything, please let me know.

[miDeb]

Thanks, I'll take a look at your PR, hopefully there aren't too many conflicts...
I think I figured out how to do it, but if you have any suggestions I'll be happy to take them.

[sylvestre]

---- test_sort::test_keys_invalid_field_zero stdout ----

current_directory_resolved: 

run: /home/travis/build/uutils/coreutils/target/debug/coreutils sort -k 0.1

thread 'test_sort::test_keys_invalid_field_zero' panicked at 'Broken pipe (os error 32)', tests/common/util.rs:742:37

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Is failing twice in the CI

[miDeb]

I haven't been able to reproduce this locally, and I have no idea why this is failing for this test specifically...

[miDeb]

Well, that seems to have worked. There's one remaining failing test, saying
Error: Incompatible 'Cargo.lock' format; try cargo +1.40.0 update, which I guess isn't something I have caused because I haven't modified Cargo.lock at all.

[sylvestre]

Yeah, don't bother about minrustv :)

[sylvestre]

I will fix it here:
#2013

[sylvestre]

Please let me know when it is ready :)

[miDeb]

This should be more or less ready. I don't know if I should wait for #2008 to be merged first so I can rebase on top of it, as there are going to be some conflicts

[sylvestre]

merged #2008 sorry for the conflicts :(
it has plenty of tests

[miDeb]

Thanks, I already expected that :)

[miDeb]

This should be ready now. Performance seems to have regressed slightly, but I don't think it's a big deal. Note that if you want to measure performance you should specify an outfile (-o), because I made writes to stdout buffered as a drive-by change, resulting in a clear perf win for redirecting stdout (sort > outfile).

[miDeb]

Spoke too soon, some tests are failing. Will fix them :)

[sylvestre]

could you please create and document how you did the benchmarking?
For example, create a
BENCHMARKING.md
file
with where to get a data set (or generate it) and the commands to evaluate performances? (with hyperfine if possible)

thanks

[miDeb]

I added a benchmarking.md file. Below is a comparison of performance before and after my change:

wordlist: (hyperfine "target/release/coreutils sort shuffled_wordlist.txt -o output.txt")
before:
Time (mean ± σ): 26.1 ms ± 3.6 ms [User: 64.3 ms, System: 9.6 ms]
after:
Time (mean ± σ): 27.2 ms ± 2.3 ms [User: 107.5 ms, System: 12.7 ms]

wordlist ignoring the case (hyperfine "target/release/coreutils sort shuffled_wordlist.txt -f -o output.txt")
before:
Time (mean ± σ): 253.4 ms ± 2.5 ms [User: 590.0 ms, System: 177.0 ms]
after:
Time (mean ± σ): 41.5 ms ± 4.0 ms [User: 142.8 ms, System: 13.0 ms]
This performance improvement is because we're uppercasing everything at the beginning once instead of at every comparison.

numbers: (hyperfine "target/release/coreutils sort shuffled_numbers.txt -n -o output.txt")
before:
Time (mean ± σ): 40.1 ms ± 3.8 ms [User: 338.7 ms, System: 9.7 ms]
after:
Time (mean ± σ): 44.6 ms ± 3.6 ms [User: 370.2 ms, System: 9.5 ms]