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

Add flags to control trailing decimal and zero in exponent form when input has one significant digit #195

Merged
merged 8 commits into from
May 18, 2023

Conversation

ianmcook
Copy link
Contributor

@ianmcook ianmcook commented May 11, 2023

This implements an improvement in how the EMIT_TRAILING_DECIMAL_POINT and EMIT_TRAILING_ZERO_AFTER_POINT flags affect This adds new flags EMIT_TRAILING_DECIMAL_POINT_IN_EXPONENTIAL and EMIT_TRAILING_ZERO_AFTER_POINT_IN_EXPONENTIAL to control the trailing decimal and zero in conversion to scientific notation when the input number has exactly one significant digit.

These flags control the behavior of ToShortest() and ToPrecision(, 1) which currently do not respect the existing flags EMIT_TRAILING_DECIMAL_POINT and EMIT_TRAILING_ZERO_AFTER_POINT in this situation. ToExponential() OTOH does respect the existing flags in this situation, and its behavior is unaffected by these new flags.

For example: when both of the new flags are activated, ToShortest(0.00001) and ToPrecision(0.00001, 1) both currently emit 1E-5, but after this PR they both emit 1.0E-5.

This change was originally made in Apache Arrow's vendored copy of double-conversion in apache/arrow#9816. The Apache Arrow developers would like to contribute this improvement upstream so that we can depend directly on upstream double-conversion. Thank you.

@google-cla
Copy link

google-cla bot commented May 11, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ianmcook
Copy link
Contributor Author

CLA is now signed.

@floitsch
Copy link
Collaborator

Thanks for the PR.
Should we make this a separate flag?
This would have two advantages:

  • users that want "5." but not "5.e3" can still have that option
  • it would be backwards compatible.

@ianmcook
Copy link
Contributor Author

Happy to make this a separate flag. Any suggestions for what to name it? 😅

@floitsch
Copy link
Collaborator

I would keep it simple:

  • EMIT_TRAILING_DECIMAL_POINT_IN_EXPONENTIAL
  • EMIT_TRAILING_ZERO_AFTER_POINT_IN_EXPONENTIAL

wdyt?

@ianmcook ianmcook changed the title Improve handling of EMIT_TRAILING_DECIMAL_POINT, EMIT_TRAILING_ZERO_AFTER_POINT Add flags to control trailing decimal and zero in exponent form when input has one significant digit May 15, 2023
@ianmcook
Copy link
Contributor Author

@floitsch PTAL

Copy link
Collaborator

@floitsch floitsch left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks.

@floitsch floitsch merged commit 4d841bf into google:master May 18, 2023
kou pushed a commit to apache/arrow that referenced this pull request Jun 9, 2023
…, remove patches (#36002)

[double-conversion v3.3.0](https://github.com/google/double-conversion/releases/tag/v3.3.0) adds a [new feature](google/double-conversion#195) that allows us to remove the special code that was added to our vendored copy of double-conversion in #9816.

This PR:
* Updates vendored double-conversion to v3.3.0
* Activates the new flags `EMIT_TRAILING_DECIMAL_POINT_IN_EXPONENTIAL` and `EMIT_TRAILING_ZERO_AFTER_POINT_IN_EXPONENTIAL`
* Removes the patches in `cpp/src/arrow/vendored/double-conversion/patches`

-----
* Closes: #35669

Authored-by: Ian Cook <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants