Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support scientific notation in parse_float_as_decimal #8494

Closed
wants to merge 7 commits into from

Conversation

Weijun-H
Copy link
Member

Which issue does this PR close?

Closes #8486

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Dec 11, 2023
@Weijun-H Weijun-H marked this pull request as draft December 11, 2023 15:09
@Weijun-H Weijun-H marked this pull request as ready for review December 11, 2023 15:10
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Weijun-H

@@ -206,6 +206,18 @@ select 123456789.0123456789012345678901234567890
statement error SQL error: ParserError\("Cannot parse 1234567890123456789012345678901234567890 as i128 when building decimal: number too large to fit in target type"\)
select -123456789.0123456789012345678901234567890

# scientific notation
query RRRR
select 1.234e2, 1.234e-2, -1.234e2, -1.234e-2
Copy link
Contributor

Choose a reason for hiding this comment

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

1.234e-2 is not -123.4 I don't think 🤔

DataFusion CLI v33.0.0
❯ select 1.234e-2;
+------------------+
| Float64(0.01234) |
+------------------+
| 0.01234          |
+------------------+

datafusion/sql/src/expr/value.rs Outdated Show resolved Hide resolved
@Weijun-H Weijun-H requested a review from alamb December 15, 2023 09:04
datafusion/sql/tests/sql_integration.rs Outdated Show resolved Hide resolved
)))
})?;

let replaced_str = big_decimal.to_string().replace('.', "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you have parsed the data into a BigDecimal, I think you could just convert to a i128 directly which would be more performant and I think potentially more correct

Perhaps you could use
https://docs.rs/bigdecimal/latest/bigdecimal/struct.BigDecimal.html#method.fractional_digit_count to get the scale

And then compute the precision and the i128 from
https://docs.rs/bigdecimal/latest/bigdecimal/struct.BigDecimal.html#method.into_bigint_and_exponent

@alamb
Copy link
Contributor

alamb commented Jan 31, 2024

I don't think this PR is waiting on feedback, so marking it as a draft. Please mark it as ready for review when it is ready for another look 🙏

@alamb alamb marked this pull request as draft January 31, 2024 21:00
Copy link

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label Apr 15, 2024
@github-actions github-actions bot closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner sqllogictest SQL Logic Tests (.slt) Stale PR has not had any activity for some time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: make parse_float_as_decimal work on scientific notaion
2 participants