Test : move tests for parse_string_decimal_native to parse_decimal #7177

himadripal · 2025-02-22T18:14:49Z

Which issue does this PR close?

Related to DataFusion should support casting strings such as "4e7" to decimal datafusion#10315
Follows up - Fix: Support for e notation using parse_decimal in string to decimal conversion #6905 ,based on the (Fix: Support for e notation using parse_decimal in string to decimal conversion #6905 (comment))

Few important consideration -

Existing string to decimal conversion uses parse_string_to_decimal_native
parse_string_to_decimal_native does not have support for e-notation
parse_string_to_decimal_native does rounding at scale, not truncate
parse_decimal an existing method has e-notation support and use elsewhere
Fix: Support for e notation using parse_decimal in string to decimal conversion #6905 added rounding support in parse_decimal
moved string to decimal conversion to use parse_decimal to get support for e-notation.

This PR is a first one to break up #6905 , this one only moves the existing parse_string_to_decimal_native tests to use parse_decimal. You can observe I changed the tests because parse_decimal does not have rounding support as is. Next PR, I'll introduce that change and change these tests again.

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

…imal

himadripal · 2025-02-22T18:27:05Z

arrow-cast/src/cast/mod.rs

                3,
            ),
-            "0.127"
+            "0.126"


parse_decimal does not support rounding yet. it does truncate instead

himadripal · 2025-02-22T18:27:34Z

arrow-cast/src/cast/mod.rs

        assert_eq!(
            Decimal128Type::format_decimal(
-                parse_string_to_decimal_native::<Decimal128Type>(".1265", 2).unwrap(),
+                parse_decimal::<Decimal128Type>(".1265", 38, 2).unwrap(),


parse_decimal does not support rounding yet. it does truncate instead

himadripal · 2025-02-22T18:27:44Z

arrow-cast/src/cast/mod.rs

                2,
            ),
-            "0.13"
+            "0.12"


parse_decimal does not support rounding yet. it does truncate instead, fix is in this PR #7179. This change is put back in the PR #7179

himadripal · 2025-02-22T18:28:42Z

arrow-cast/src/cast/decimal.rs

        );

+        // `parse_decimal` does not handle scale=0 correctly. will enable it as part of code change PR.
+        // assert_eq!(parse_decimal::<Decimal128Type>("123.45", 38, 0)?, 123_i128);


parse_decimal does not behave correctly when scale=0 when there are decimal digits in the original string. fix is in this PR #7179

alamb

Thank you @himadripal -- i took a brief look and have a few questions

I am sorry for the delay in reviewing and the extra review, but I don't want to introduce another regression / need to do another release

alamb · 2025-02-26T16:33:31Z

arrow-cast/src/cast/decimal.rs


-        assert_eq!(
-            parse_string_to_decimal_native::<Decimal128Type>("123.4567891", 0)?,
+        //scale = 0 is not handled correctly in parse_decimal, next PR will fix it and enable this.


Doens't this comment out existing tests? Why would you reduce test coverage?

So, this is commented as parse_decimal do not have handling of scale 0 well, Fix for scale 0 in parse_decimal is here #7179 and this test is enabled back there - all of these changes are all together in #6905

But my point is that this PR actually changes what is tested (it isn't just a migration / refactor).
I expect a refactor to change how the code is structured but not change the coverage

Now, we can close this PR, as I added additional test in #7179 to check the values generated by parse_string_to_decimal_native and parse_decimal are same. So no change in existing test or coverage.

alamb · 2025-02-26T16:35:27Z

arrow-cast/src/cast/mod.rs

        assert_eq!(
            Decimal128Type::format_decimal(
-                parse_string_to_decimal_native::<Decimal128Type>("123.45", 2).unwrap(),
+                parse_decimal::<Decimal128Type>("123.45", 38, 2).unwrap(),


This test is now testing a different function. I am not familar with the difference betwen parse_string_to_decimal and parse_decima

Idea is to move from parse_string_to_decimal_native and to use parse_decimal while doing string to decimal casting here. Main reason is that parse_decimal has support for e-notation and is also being used in readers like (i.e arrow-csv, arrow-json and also performant, some discussion here

so is your plan to remove the parse_string_to_decimal function?

Yes, to be consistent, readers and cast will be using same function.

alamb · 2025-03-17T19:08:54Z

Now, we can close this PR, as I added additional test in #7179 to check the values generated by parse_string_to_decimal_native and parse_decimal are same. So no change in existing test or coverage.

I think this means to close this PR so I am doing so. Please reopen if that was not intended

Also in the future please close any PR that you don't think needs review as that will make it easier to figure out what needs reviewing

himadripal · 2025-03-18T03:49:11Z

Now, we can close this PR, as I added additional test in #7179 to check the values generated by parse_string_to_decimal_native and parse_decimal are same. So no change in existing test or coverage.

I think this means to close this PR so I am doing so. Please reopen if that was not intended

Also in the future please close any PR that you don't think needs review as that will make it easier to figure out what needs reviewing

Because it was one part of the break up PR, I waiting for confirmation before I could close.

himadripal added 2 commits February 22, 2025 09:49

change test to use parse decimal

746387a

move the mod.rs tests for parse_string_to_decimal_native to parse_dec…

7738768

…imal

github-actions bot added the arrow Changes to the arrow crate label Feb 22, 2025

revert the code change, only test move, comment the failed test.

359a5ec

himadripal commented Feb 22, 2025

View reviewed changes

himadripal mentioned this pull request Feb 22, 2025

Fix: Support for e notation using parse_decimal in string to decimal conversion #6905

Closed

disable a test for scale=0

296e0fd

himadripal mentioned this pull request Feb 23, 2025

feat: add rounding logic and scale zero fix parse_decimal to match parse_string_to_decimal_native behavior #7179

Draft

alamb mentioned this pull request Feb 26, 2025

Weekly Plan (Andrew Lamb) Feb 24, 2025 apache/datafusion#14850

Closed

10 tasks

alamb reviewed Feb 26, 2025

View reviewed changes

alamb mentioned this pull request Mar 3, 2025

Weekly Plan (Andrew Lamb) March 3, 2025 apache/datafusion#14978

Closed

12 tasks

alamb closed this Mar 17, 2025

Test : move tests for parse_string_decimal_native to parse_decimal #7177

Test : move tests for parse_string_decimal_native to parse_decimal #7177

Uh oh!

Conversation

himadripal commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

himadripal Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 22, 2025

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

himadripal Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

himadripal Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 17, 2025

Uh oh!

himadripal commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

himadripal commented Feb 22, 2025 •

edited

Loading

himadripal Feb 22, 2025 •

edited

Loading

himadripal Feb 22, 2025 •

edited

Loading

himadripal Feb 22, 2025 •

edited

Loading

himadripal Feb 26, 2025 •

edited

Loading

himadripal Feb 27, 2025 •

edited

Loading

himadripal Feb 26, 2025 •

edited

Loading