Skip to content

Fix forder-order equivalence test for atypical collate locales - #7837

Open
MichaelChirico wants to merge 5 commits into
masterfrom
fix-latvian-tests
Open

Fix forder-order equivalence test for atypical collate locales#7837
MichaelChirico wants to merge 5 commits into
masterfrom
fix-latvian-tests

Conversation

@MichaelChirico

@MichaelChirico MichaelChirico commented Jul 24, 2026

Copy link
Copy Markdown
Member

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 Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.01%. Comparing base (dcaa87c) to head (e40fca4).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7837   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files          88       88           
  Lines       17286    17286           
=======================================
  Hits        17116    17116           
  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.

@MichaelChirico
MichaelChirico changed the base branch from test-err-show-diff to other-attach-order July 26, 2026 17:03
@MichaelChirico
MichaelChirico force-pushed the fix-latvian-tests branch 3 times, most recently from 9548bbd to 4c6d0a2 Compare July 27, 2026 07:09
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
  • HEAD=fix-latvian-tests slower P<0.001 for fwrite refactored in #6393
    Comparison Plot

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

MichaelChirico commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

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.

@MichaelChirico
MichaelChirico changed the base branch from other-attach-order to old-nchar-try July 27, 2026 20:41
@MichaelChirico
MichaelChirico force-pushed the fix-latvian-tests branch 2 times, most recently from d7cf097 to 5b56eb4 Compare July 28, 2026 02:04
@MichaelChirico
MichaelChirico force-pushed the old-nchar-try branch 2 times, most recently from 401eb10 to c4a4fee Compare July 28, 2026 05:37
@MichaelChirico
MichaelChirico force-pushed the fix-latvian-tests branch 2 times, most recently from 16f7a09 to 417989b Compare July 28, 2026 15:02
Base automatically changed from old-nchar-try to master July 28, 2026 20:09
@MichaelChirico
MichaelChirico force-pushed the fix-latvian-tests branch 3 times, most recently from de8913a to 096e827 Compare July 28, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant