join: report the field number the user gave - #13995
Open
MsfPablo wants to merge 1 commit into
Open
Conversation
Two ways the 'incompatible join fields' message diverged from its input. parse_field_number clamps an out-of-range field to usize::MAX, and get_field_number rendered it one-based with k + 1: an abort under overflow-checks, a wrap to 0 otherwise. Saturate instead. Separately, translate! stringifies a value and re-parses it as i64 or f64 before handing it to Fluent, whose number type is f64-backed. Every integer above 2^53 was silently rounded. Route those through as exact decimal strings, keeping the number path for values f64 holds exactly so plural rules and number formatting are unaffected. Fixes uutils#13376
|
GNU testsuite comparison: |
Merging this PR will regress 2 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | numfmt_large_numbers_si[10000] |
93.2 ms | 98.4 ms | -5.34% |
| ❌ | Simulation | ptx_input_references_long_lines[1000] |
1.9 s | 2 s | -4.31% |
| ⚡ | Simulation | shuf_lines[100000] |
72.1 ms | 54.7 ms | +31.77% |
| ⚡ | Simulation | shuf_repeat_sampling[50000] |
14 ms | 12.3 ms | +14.18% |
| ⚡ | Simulation | expand_many_lines[100000] |
116.4 ms | 107.3 ms | +8.55% |
| ⚡ | Simulation | numfmt_stream_to_si_precision |
390.9 ms | 361.1 ms | +8.25% |
| ⚡ | Simulation | expand_custom_tabstops[50000] |
31.5 ms | 29.2 ms | +7.93% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing MsfPablo:join-exact-field-numbers (a7b3dee) with main (9504124)
Footnotes
-
51 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13376.
Both halves of the issue come down to the reported field number not matching the input.
The
k + 1overflow.parse_field_numberdeliberately clamps an out-of-range field tousize::MAX, andget_field_numberthen renders it one-based withk + 1. That aborts under overflow-checks and wraps to0in a normal build. Now saturates.The rounding. This one is not join-specific, so I fixed it where it lives.
translate!stringifies a value, re-parses it asi64orf64, and sets it on Fluent as a number — butFluentNumberis f64-backed, so anything past 2^53 rounds. Note that taking thei64branch is not enough to stay exact;-j 9007199254740993parses fine asi64and still comes out as…992. So the check is whether f64 can hold the value exactly, not whetheri64can:That also fixes the
csplitcase you mentioned:All the values from the issue now match GNU, including
18446744073709551615for the clamped case.Tests: unit tests in
locale.rsfor the representability boundary, and an integration test intest_join.rscovering the four field values above.Since this touches
uucore, I ran the suites for csplit, nl, split, head, expr, seq, tail, wc, fold and sort as well as join — 982 tests, all passing.