Skip to content

Schubfach #1079

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Schubfach #1079

wants to merge 7 commits into from

Conversation

zihaowang678
Copy link

Overview

This PR adds an implementation of the Schubfach algorithm for efficient and accurate float-to-string conversion in ion-java. The Schubfach algorithm provides performance improvements over traditional approaches while maintaining correctness.

Changes

  • Added DoubleToDecimal.java which implements the core Schubfach algorithm for double values
  • Added MathUtils.java to provide mathematical functions specifically designed for the Schubfach algorithm
  • Integrated with the existing Ion Java codebase for floating-point serialization

Benefits

  • Performance comparable to or faster than the native Double.toString() method in JDK 21
  • Consistent serialized size with runtime JDK 21; reduced serialized size with runtime JDK 17 and lower versions
  • Greatly improved memory allocation efficiency for JDK 17 and lower versions
  • Guaranteed shortest correctly rounded string representation

References

  • Original paper: "The Schubfach way to render doubles" by Raffaello Giulietti
  • Implementation based on the reference Java version with adaptations for Ion's requirements

License

The implementation maintains the original MIT license from Raffaello Giulietti's reference implementation.

Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 77.72277% with 45 lines in your changes missing coverage. Please review.

Project coverage is 67.93%. Comparing base (3c1b6b1) to head (f3a5e44).
Report is 111 commits behind head on master.

Files with missing lines Patch % Lines
...com/amazon/ion/impl/schubfach/DoubleToDecimal.java 75.82% 31 Missing and 13 partials ⚠️
.../java/com/amazon/ion/impl/schubfach/MathUtils.java 94.44% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1079      +/-   ##
============================================
+ Coverage     67.23%   67.93%   +0.69%     
- Complexity     5484     5608     +124     
============================================
  Files           159      162       +3     
  Lines         23025    23281     +256     
  Branches       4126     4165      +39     
============================================
+ Hits          15481    15815     +334     
+ Misses         6262     6179      -83     
- Partials       1282     1287       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zihaowang678 zihaowang678 marked this pull request as ready for review July 11, 2025 21:54
Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

Nice investigation!

Comment on lines 501 to 505
}
if (-3 < e && e <= 0) {
} else if (-3 < e && e <= 0) {
return toChars2(h, m, l, e);
} else {
return toChars3(h, m, l, e);
}
return toChars3(h, m, l, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks unrelated. If that's true, then I generally recommend avoiding including changes like this because it just increases the size of the diff and distracts the reviewer. Non-functional changes are often good, but they should go in standalone PRs when practical.

In this case though, I think the ways of writing this are up to personal preference.

Copy link
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

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

It's great to fold the string customizations in so that we don't have to do string inspection and modification in post. We have to tweak the special values to be spec-compliant for Ion though.

Comment on lines 269 to 271
case PLUS_INF: return "Infinity";
case MINUS_INF: return "-Infinity";
default: return "NaN";
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be +inf, -inf and nan respectively: https://amazon-ion.github.io/ion-docs/docs/float.html#special-values

Comment on lines 269 to 271
case PLUS_INF: return "Infinity";
case MINUS_INF: return "-Infinity";
default: return "NaN";
Copy link
Contributor

Choose a reason for hiding this comment

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

See Ion Floats: Special Values.

Suggested change
case PLUS_INF: return "Infinity";
case MINUS_INF: return "-Infinity";
default: return "NaN";
case PLUS_INF: return "+inf";
case MINUS_INF: return "-inf";
default: return "nan";

Comment on lines 294 to 296
case PLUS_INF: return app.append("Infinity");
case MINUS_INF: return app.append("-Infinity");
default: return app.append("NaN");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above:

Suggested change
case PLUS_INF: return app.append("Infinity");
case MINUS_INF: return app.append("-Infinity");
default: return app.append("NaN");
case PLUS_INF: return "+inf";
case MINUS_INF: return "-inf";
default: return "nan";

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.

3 participants