You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "iabhi4 (via GitHub)" <gi...@apache.org> on 2025/05/26 20:52:57 UTC
[PR] ARROW-46589: Fixed utf8_is_digit to support full Unicode digit range [arrow]
iabhi4 opened a new pull request, #46590:
URL: https://github.com/apache/arrow/pull/46590
### Rationale for this change
Closes #46589
`pyarrow.compute.utf8_is_digit` did not recognize valid Unicode digit characters (e.g., superscripts like `'³'`), diverging from the behavior of Python's built-in `str.isdigit()`
This caused inconsistencies in downstream libraries like pandas when using PyArrow-backed StringDtype.
### What changes are included in this PR?
Updated `IsDigitCharacterUnicode` implementation to cover a broader range of Unicode digits by replacing category check with one that aligns with Python’s `str.isdigit()` semantics.
Added tests in `scalar_string_test.cc` to validate correct digit detection across diverse Unicode digit inputs.
### Are these changes tested?
Yes. New unit tests were added and pass successfully, verifying behavior on various Unicode digit characters.
### Are there any user-facing changes?
Yes, users relying on `pc.utf8_is_digit()` will now get correct results for a wider range of Unicode digit characters, improving correctness and parity with Python semantics
- GitHub Issue: [#46589](https://github.com/apache/arrow/issues/46589)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2107950177
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -2640,4 +2640,10 @@ TEST(TestStringKernels, UnicodeLibraryAssumptions) {
}
#endif
+TEST(TestUtf8IsDigit, SupportsUnicodeDigits) {
+ auto input = ArrayFromJSON(utf8(), R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])");
+ auto expected = ArrayFromJSON(boolean(), "[true, true, true, false, true, true, false, true]");
+ CheckScalarUnary("utf8_is_digit", input, expected);
+}
Review Comment:
```suggestion
TEST(TestUtf8IsDigit, SupportsUnicodeDigits) {
auto input = ArrayFromJSON(utf8(), R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])");
auto expected = ArrayFromJSON(boolean(), "[true, true, true, false, true, true, false, true]");
CheckScalarUnary("utf8_is_digit", input, expected);
}
#endif
```
##########
cpp/src/arrow/compute/kernels/scalar_string_utf8.cc:
##########
@@ -140,7 +140,7 @@ static inline bool IsDecimalCharacterUnicode(uint32_t codepoint) {
static inline bool IsDigitCharacterUnicode(uint32_t codepoint) {
// Python defines this as Numeric_Type=Digit or Numeric_Type=Decimal.
// utf8proc has no support for this, this is the best we can do:
Review Comment:
Should we update this comment?
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -2640,4 +2640,10 @@ TEST(TestStringKernels, UnicodeLibraryAssumptions) {
}
#endif
+TEST(TestUtf8IsDigit, SupportsUnicodeDigits) {
Review Comment:
```suggestion
TEST(TestStringKernels, Utf8IsDigit) {
```
BTW, should we update existing (but empty) one instead of adding a new test?
https://github.com/apache/arrow/blob/19439118ce25620b9773fe3616aa1e4dab4fd098/cpp/src/arrow/compute/kernels/scalar_string_test.cc#L1386-L1391
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "iabhi4 (via GitHub)" <gi...@apache.org>.
iabhi4 commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2110798927
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1384,10 +1384,12 @@ TYPED_TEST(TestStringKernels, IsDecimalUnicode) {
}
TYPED_TEST(TestStringKernels, IsDigitUnicode) {
- // These are digits according to Python, but we don't have the information in
- // utf8proc for this
- // this->CheckUnary("utf8_is_digit", "[\"²\", \"①\"]", boolean(), "[true,
- // true]");
+ // Tests for digits across various Unicode scripts.
+ // ٤: Arabic 4, ³: Superscript 3, ५: Devanagari 5, Ⅷ: Roman 8 (not digit), １２３: Fullwidth 123
+ this->CheckUnary("utf8_is_digit",
+ R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])",
+ boolean(),
+ "[true, true, true, false, true, true, false, true]");
}
TYPED_TEST(TestStringKernels, IsNumericUnicode) {
Review Comment:
Hi @pitrou,
Thanks for the suggestion to expand the test, it helped uncover a subtle issue. While testing characters in `UTF8PROC_CATEGORY_NO`, I noticed that values like `'¾'` (vulgar fraction) return true, even though Python's `str.isdigit()` returns false.
Most edge cases behave as expected, but this seems to be an inconsistency due to `utf8proc` treating all No characters as numeric. Would it make sense to document this in a comment as a known limitation of `utf8proc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2108271467
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1384,10 +1384,12 @@ TYPED_TEST(TestStringKernels, IsDecimalUnicode) {
}
TYPED_TEST(TestStringKernels, IsDigitUnicode) {
- // These are digits according to Python, but we don't have the information in
- // utf8proc for this
- // this->CheckUnary("utf8_is_digit", "[\"²\", \"①\"]", boolean(), "[true,
- // true]");
+ // Tests for digits across various Unicode scripts.
+ // ٤: Arabic 4, ³: Superscript 3, ५: Devanagari 5, Ⅷ: Roman 8 (not digit), １２３: Fullwidth 123
+ this->CheckUnary("utf8_is_digit",
+ R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])",
+ boolean(),
+ "[true, true, true, false, true, true, false, true]");
}
TYPED_TEST(TestStringKernels, IsNumericUnicode) {
Review Comment:
Can we perhaps update this test with more values as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fix utf8_is_digit to support full Unicode digit range [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #46590:
URL: https://github.com/apache/arrow/pull/46590
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "iabhi4 (via GitHub)" <gi...@apache.org>.
iabhi4 commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2107992375
##########
cpp/src/arrow/compute/kernels/scalar_string_utf8.cc:
##########
@@ -140,7 +140,7 @@ static inline bool IsDecimalCharacterUnicode(uint32_t codepoint) {
static inline bool IsDigitCharacterUnicode(uint32_t codepoint) {
// Python defines this as Numeric_Type=Digit or Numeric_Type=Decimal.
// utf8proc has no support for this, this is the best we can do:
Review Comment:
I updated the comment. Let me know if you want more information on there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "iabhi4 (via GitHub)" <gi...@apache.org>.
iabhi4 commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2110798927
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1384,10 +1384,12 @@ TYPED_TEST(TestStringKernels, IsDecimalUnicode) {
}
TYPED_TEST(TestStringKernels, IsDigitUnicode) {
- // These are digits according to Python, but we don't have the information in
- // utf8proc for this
- // this->CheckUnary("utf8_is_digit", "[\"²\", \"①\"]", boolean(), "[true,
- // true]");
+ // Tests for digits across various Unicode scripts.
+ // ٤: Arabic 4, ³: Superscript 3, ५: Devanagari 5, Ⅷ: Roman 8 (not digit), １２３: Fullwidth 123
+ this->CheckUnary("utf8_is_digit",
+ R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])",
+ boolean(),
+ "[true, true, true, false, true, true, false, true]");
}
TYPED_TEST(TestStringKernels, IsNumericUnicode) {
Review Comment:
Hi @pitrou,
Thanks for the suggestion to expand the test — it helped uncover a subtle issue. While testing characters in `UTF8PROC_CATEGORY_NO`, I noticed that values like `'¾'` (vulgar fraction) return true, even though Python's `str.isdigit()` returns false.
Most edge cases behave as expected, but this seems to be an inconsistency due to `utf8proc` treating all No characters as numeric. Would it make sense to document this in a comment as a known limitation of `utf8proc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2111223493
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1384,10 +1384,12 @@ TYPED_TEST(TestStringKernels, IsDecimalUnicode) {
}
TYPED_TEST(TestStringKernels, IsDigitUnicode) {
- // These are digits according to Python, but we don't have the information in
- // utf8proc for this
- // this->CheckUnary("utf8_is_digit", "[\"²\", \"①\"]", boolean(), "[true,
- // true]");
+ // Tests for digits across various Unicode scripts.
+ // ٤: Arabic 4, ³: Superscript 3, ५: Devanagari 5, Ⅷ: Roman 8 (not digit), １２３: Fullwidth 123
+ this->CheckUnary("utf8_is_digit",
+ R"(["٤", "³", "५", "Ⅷ", "٣٣", "१२३", "abc", "１２３"])",
+ boolean(),
+ "[true, true, true, false, true, true, false, true]");
}
TYPED_TEST(TestStringKernels, IsNumericUnicode) {
Review Comment:
I think we can a comment stating this indeed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fix utf8_is_digit to support full Unicode digit range [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #46590:
URL: https://github.com/apache/arrow/pull/46590#issuecomment-3638598598
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit dc0f5a9415aa3d2c5dc4f226f51a7e03e9c71ccd.
None of the specified runs were found on the Conbench server.
The [full Conbench report](https://github.com/apache/arrow/runs/57706158947) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ARROW-46589: Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46590:
URL: https://github.com/apache/arrow/pull/46590#issuecomment-2910614195
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "iabhi4 (via GitHub)" <gi...@apache.org>.
iabhi4 commented on code in PR #46590:
URL: https://github.com/apache/arrow/pull/46590#discussion_r2107991626
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -2640,4 +2640,10 @@ TEST(TestStringKernels, UnicodeLibraryAssumptions) {
}
#endif
+TEST(TestUtf8IsDigit, SupportsUnicodeDigits) {
Review Comment:
> BTW, should we update existing (but empty) one instead of adding a new test?
Thanks for pointing that out, I updated the test in the existing block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46589: [C++] Fixed utf8_is_digit to support full Unicode digit range [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #46590:
URL: https://github.com/apache/arrow/pull/46590#issuecomment-2930464888
I pushed a quick fix for the C++ lint failures, will merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org