Skip to content

Read decimal digit values from the bundled character database - #8748

Merged
youknowone merged 1 commit into
RustPython:mainfrom
zzarbttoo:char-to-decimal-from-ucd
Sep 20, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
zzarbttoo:char-to-decimal-from-ucd

Conversation

@zzarbttoo

@zzarbttoo zzarbttoo commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

One of checkbox below must be checked.

  • I did not use AI to write the code of this patch.
  • This PR follows our AI policy

Summary

char_to_decimal in crates/common/src/str.rs carried its own sorted table of 590 decimal digit characters. It was added in #5643 because common had no way to reach the Unicode numeric data at the time — that data lived in stdlib, which sits above common in the dependency graph. The table's own comment said as much:

// TODO: this should probably live in a crate like unic or unicode-properties

#8211 extracted that data into rustpython-unicode, which sits below common, and made common depend on it. So the table can now go, in favour of Ucd::decimal — the same source unicodedata.decimal consults.

pub fn char_to_decimal(ch: char) -> Option<u8> {
    let value = rustpython_unicode::Ucd::new(true).decimal(CodePoint::from(ch))?;
    u8::try_from(value).ok()
}

This fixes a CPython incompatibility

The hand-written table was last updated for Unicode 10. The bundled UCD is Unicode 17, which has 180 more decimal digits across 18 scripts — Hanifi Rohingya, Wancho, Tangsa, Ol Onal, Kirat Rai, segmented digits and others. int() and float() rejected all of them, even though unicodedata.decimal in the same interpreter knew their value:

>>> int('𐴳')                      # U+10D33 HANIFI ROHINGYA DIGIT THREE
ValueError: invalid literal for int() with base 10: '𐴳'
>>> unicodedata.decimal('𐴳')
3

CPython accepts int('𐴳') and returns 3. After this patch RustPython does too, and the two lookups can no longer drift apart on a future Unicode update.

All 590 characters the old table covered keep their previous value (verified by diffing the table against the bundled UCD: 180 gained, 0 changed, 0 lost), so nothing that already worked changes.

Performance

About 1.5ns more per non-ASCII character, since it reads a property trie and a value table rather than binary-searching one 2.4KB array — int() over a 4000-digit non-ASCII string goes from 12.2 to 13.6 ns/char. ASCII input never reaches it: the sole caller, transform_decimal_and_space_to_ascii, returns early for ASCII strings, so ordinary numeric parsing is unaffected (5.6 → 5.4 ns/char, i.e. noise).

Tests

Adds char_to_decimal_values, covering ASCII, an Arabic-Indic digit the old table had, two digits it predated (U+10D35, U+1FBF5), and non-decimal characters (½, 가).

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@github-actions github-actions Bot added the z-ca-2026 Tag to track Contribution Academy 2026 label Sep 20, 2026
@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 473c6146-43af-4923-b7ed-de7cc8cb3121

📥 Commits

Reviewing files that changed from the base of the PR and between 6c5d118 and d9ede6d.

📒 Files selected for processing (1)
  • crates/common/src/str.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

char_to_decimal now reads decimal values from the bundled Unicode database. Tests cover digits from multiple scripts and reject non-decimal numeric and unrelated characters.

Changes

Unicode decimal lookup

Layer / File(s) Summary
Database lookup and validation
crates/common/src/str.rs
char_to_decimal replaces the manual digit table with a Unicode database lookup. The tests cover ASCII and additional Unicode scripts, and return None for non-decimal numerics and unrelated characters.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: using the bundled character database to read decimal digit values.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zzarbttoo
zzarbttoo force-pushed the char-to-decimal-from-ucd branch from 6c5d118 to d9ede6d Compare September 20, 2026 05:22
@zzarbttoo
zzarbttoo marked this pull request as draft September 20, 2026 05:24
`char_to_decimal` carried its own sorted table of 590 decimal digit
characters, added in RustPython#5643 because `common` had no way to reach the
Unicode numeric data at the time: it lived in `stdlib`, which sits above
`common` in the dependency graph. The table's own comment said as much —
"this should probably live in a crate like unic or unicode-properties".

RustPython#8211 extracted that data into `rustpython-unicode`, below `common`, and
made `common` depend on it, so the table can now be dropped in favour of
`Ucd::decimal` — the same source `unicodedata.decimal` consults.

This fixes a CPython incompatibility. The table was last updated for
Unicode 10; the bundled UCD is Unicode 17, which has 180 more decimal
digits across 18 scripts (Hanifi Rohingya, Wancho, Tangsa, Ol Onal,
segmented digits and others). Those were rejected by `int()` and
`float()` even though `unicodedata.decimal` knew their value:

    >>> int('\u{10D33}')        # HANIFI ROHINGYA DIGIT THREE
    ValueError                  # CPython: 3
    >>> unicodedata.decimal('\u{10D33}')
    3

All 590 characters the table covered keep their previous value, so
nothing that already worked changes, and the two lookups can no longer
drift apart on a future Unicode update.

The lookup costs about 1.5ns more per non-ASCII character, since it reads
a property trie and a value table instead of binary-searching one 2.4KB
array: `int()` over a 4000-digit non-ASCII string goes from 12.2 to
13.6 ns/char. ASCII input never reaches it — the sole caller,
`transform_decimal_and_space_to_ascii`, returns early for ASCII strings —
so ordinary numeric parsing is unaffected.

Assisted-by: Claude Code:claude-opus-5
@zzarbttoo
zzarbttoo force-pushed the char-to-decimal-from-ucd branch from d9ede6d to 18b9b5b Compare September 20, 2026 05:32
@zzarbttoo
zzarbttoo marked this pull request as ready for review September 20, 2026 05:46
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@zzarbttoo
zzarbttoo marked this pull request as draft September 20, 2026 06:31

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you caught a good point, thanks!

@zzarbttoo
zzarbttoo marked this pull request as ready for review September 20, 2026 07:17
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@youknowone
youknowone merged commit 587880b into RustPython:main Sep 20, 2026
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants