Fix forder-order equivalence test for atypical collate locales

[MichaelChirico]

Running test.data.table() in an LC_ALL=lv_LV.utf8 session can error like:

...Sys.getlocale()=='...LC_COLLATE=lv_LV.utf8...'...
Error in test.data.table() : 
  387 errors out of 12808. Search tests/tests.Rraw for test numbers 1223.007, 1223.011, 1223.021, 1223.022, 1223.029, 1223.03, 1223.049, 1223.05, 1223.057, 1223.058, 1223.061, 1223.063, 1223.065, 1223.066, 1223.067, 1223.069, 1223.07, 1223.121, 1223.122, 1223.123, 1223.124, 1223.129, 1223.13, 1223.133, 1223.134, 1223.137, 1223.138, 1223.139, 1223.14, 1223.141, 1223.142, 1223.201, 1223.202, 1223.205, 1223.206, 1223.209, 1223.21, 1223.211, 1223.212, 1223.213, 1223.214, 1223.225, 1223.226, 1223.227, 1223.228, 1223.229, 1223.231, 1223.377, 1223.378, 1223.381, 1223.382, 1223.385, 1223.386, 1223.389, 1223.39, 1223.457, 1223.458, 1223.461, 1223.462, 1223.465, 1223.466, 1223.469, 1223.47, 1252.017, 1252.018, 1252.019, 1252.02, 1252.045, 1252.046, 1252.047, 1252.048, 1252.049, 1252.05, 1252.051, 1252.052, 1252.069, 1252.07, 1252.071, 1252.072, 1252.073, 1252.074, 1252.075, 1252.076, 1252.085, 1252.086, 1252.087, 1252.088, 1252.089, 1252.09, 1252.091, 1252.092, 1252.093, 1252.094, 1252.09

It's all down to xtfrm(letters) not being able to support C-collated sorting, so using it in a locale where sort(letters) differs from ASCII causes issues.

In 268c7d6, I tried just using order(method='radix') as much as possible, but tests 1252.* explicitly guard against doing that given it's kinda pointless to test our implementation against... whatever drift has happened between our implementations since our algorithm was first merged into {base}.

So here, I take the approach to avoid using all of letters in such cases, instead focusing on the C-sortable subset of letters (see comment below for more nuance on this).

According to Gemini, the three most adversarial locales for this exercise are lv_LV, lt_LT, and az_AZ, each of which leave only 11 letters in the chosen algorithm. I confirm that result, but it doesn't rule out there being some other more adversarial locale. Gemini also mentions haw_US and sm_WS could leave 9, but it seems like in practice those locales just use normal ASCII sorting.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.01%. Comparing base (7bc8bc1) to head (8f7db24).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7837   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files          88       88           
  Lines       17292    17292           
=======================================
  Hits        17122    17122           
  Misses        170      170           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

• HEAD=fix-latvian-tests slower P<0.001 for fwrite refactored in #6393
  

Generated via commit 096e827

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	2 minutes and 44 seconds
Installing different package versions	1 minutes and 34 seconds
Running and plotting the test cases	5 minutes and 26 seconds

[MichaelChirico]

The approach with c_sortable_letters still didn't quite WAI, because sorting in lv_LV is pretty complicated -- i and y are treated as the same except in the case of a tie-breaker, AIUI:

sort(c('ib', 'ya'))
# [1] "ya" "ib"
sort(c('i', 'y'))
# [1] "i" "y"

i.e. there's a mix of right-to-left and left-to-right logic to determine sort order.

Now, I am trying c_sortable_pairs which instead uses the same approach with pairs of letters to try and mitigate this issue.

[aitap]

Tried this:

# collations.mk
LOCALES = $(shell locale -a | sed 's/[.].*//' | sort -u | grep -v -e '^C$' -e '^POSIX$')
OUTPUTS = $(foreach locale,$(LOCALES),LC_COLLATE_$(locale).out)
R = R

all: $(OUTPUTS)

LC_COLLATE_%.out:
	LC_COLLATE=$* R_LIBS_USER=data.table.Rcheck $(R)script -e 'library(data.table); test.data.table()' >$@ 2>&1 || true

sudo apt install locales-all
R CMD INSTALL -l data.table.Rcheck $whatever
OMP_THREAD_LIMIT=1 make -f collations.mk -j`nproc`

With 1.18.4 I get:

$ tail -q -n 1 *.out | perl -lpe "s/completed ok\K in.*//" | sort | uniq -c
     46 All 12100 tests (last 2368.3) in tests/tests.Rraw.bz2 completed ok
    290 All 12101 tests (last 2368.3) in tests/tests.Rraw.bz2 completed ok
     15 Execution halted

(probably should've tried with master instead)

and with fix-latvian-tests I get:

$ tail -q -n 1 *.out | perl -lpe "s/completed ok\K in.*//" | sort | uniq -c
    341 All 12822 tests (last 2381.3) in tests/tests.Rraw completed ok
     10 Execution halted

collations.zip

The sets of failed tests are quite interesting. Here are their sizes and the sizes of their set difference:

      CRAN fix-latvian-tests CRAN minus fix fix minus CRAN
az_AZ  658                 1            657              0
az_IR  658                 1            657              0
cy_GB   22                 1             21              0
da_DK    1               325              0            324
et_EE  729                 0            729              0
fo_FO    1                 1              0              0
fy_DE  441                 0            441              0
fy_NL  441                 0            441              0
hr_HR  243                 0            243              0
hu_HU    0               324              0            324
lt_LT  431                 0            431              0
lv_LV  729                 0            729              0
nb_NO   46                 1             45              0
nn_NO    1               325              0            324
om_KE   63                 0             63              0
sq_AL    0               228              0            228
sq_MK   13                 9             12              8

• et_EE, fy_DE, fy_NL, hr_HR, lt_LT, lv_LV, om_KE are completely fixed.
• az_AZ, az_IR: 1913.07 remains broken
• cy_GB, nb_NO, fo_FO: 355 remains broken
• da_DK, nn_NO have 324 newly broken 1252.* tests, 355 remains broken
• hu_HU, sq_AL: 1252.* tests are newly broken
• sq_MK: some 1223.* tests are fixed, but others are newly broken

In 1913.07, the difference is in row order, probably because merge.data.frame sorted the strings in locale order. In 355, the orders of levels are different (again because is.unsorted(newlevels) is TRUE according to locale rules but data.table:::forderv(newlevels) returns an empty vector); since all.equal(as.character(DT$f), as.character(desired$f)) is TRUE, they represent the same data.

So why do tests 1223.*, 1252.* still break? Reducing an example, I get:

Sys.setlocale('LC_COLLATE', 'hu_HU')
s <- c("pzvz", "pzsz")
order(s)
# [1] 1 2
data.table:::forderv(s)
# [1] 2 1
c('pz', 'vz', 'sz', 'zs') %in% c_sortable_pairs
# [1]  TRUE  TRUE  TRUE FALSE

While the strings have been constructed from c_sortable_pairs, they also contain a letter pair that sorts differently in the hu_HU locale and byte-wise. Maybe you could break these unintended pairs by dropping the collapse="" argument when generating strings from them? This would also avert the potential problem of tri- and tetra-grams influencing the sorting order.

[MichaelChirico]

Excellent. I wonder how many locales it's worth including in the matrix for other.Rraw. I could add az_AZ, hu_HU, fo_FO, and sq_MK based on your table. Is it overkill?

Maybe you could break these unintended pairs by dropping the collapse="" argument when generating strings from them?

I think you mean to use collapse = " " instead (CMIIW)... makes some sense, though I wonder if the purpose of the test is to test sorting "words" where restricting ourselves to bigrams is maybe "cheating". OTOH, maybe we're going too far down the rabbit hole here -- in all cases for 1223/1252, data.table is working as intended, and it's just the base comparison that's "broken".

(of course, I do find the exercise a bit "fun" so am happy to tilt at the windmill 😃)

[aitap]

If you would like to save some CI time, you could merge da_DK, nn_NO, hu_HU, sq_AL, sq_MK into a single class because it's probably the same kind of failure and pick da_DK or nn_NO because they also exercise 355. At least for some locales it's a coin toss whether 1252 or 1223 (or both) will fail this time; 1252 seems to be more likely. (It was puzzling to see sq_MK and sq_AL showing different failures despite sq_MK importing collation sequences from sq_AL.) Then fo_FO could be removed, leaving az_AZ and da_DK. On the other hand, if the test runs monthly, why not make it comprehensive and include all four locales?

I think you mean to use collapse = " " instead (CMIIW)...

Exactly!

I wonder if the purpose of the test is to test sorting "words" where restricting ourselves to bigrams is maybe "cheating".

What are the alternatives? Generating "words" using some sort of Markov algorithm that always produces good bigrams? Wrapping all sorting tests in Sys.setlocale('LC_COLLATE','C') instead of just a few? Forcing sorting tests to use order(method="radix")? Only one of these sounds fun to implement.