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

[FLINK-4268] [core] Add parsers for BigDecimal/BigInteger #2304

Closed
wants to merge 2 commits into from

Conversation

twalthr
Copy link
Contributor

@twalthr twalthr commented Jul 27, 2016

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General
    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation
    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build
    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed

This PR is similar to FLINK-4248. It adds parsers for the basic types BigDecimal and BigInteger.

return -1;
}

String str = new String(bytes, startPos, i - startPos);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating String instances, we could copy the relevant bytes into a reusable char[] and create the BigDecimal with the BigDecimal(char[] in, int offset, int len) constructor.

@fhueske
Copy link
Contributor

fhueske commented Aug 19, 2016

Thanks @twalthr, PR looks good.
Only added a hint how String creations can be avoided.

Good to merge otherwise.

@twalthr twalthr force-pushed the FLINK-4268 branch 2 times, most recently from 057af26 to c9fde0e Compare September 19, 2016 16:25
@twalthr
Copy link
Contributor Author

twalthr commented Sep 19, 2016

@fhueske I improved the big decimal parsing. Now less objects are created. I hope I didn't introduce new bugs.

Copy link
Contributor

@fhueske fhueske left a comment

Choose a reason for hiding this comment

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

Thanks for the update @twalthr. I added a few comments to improve the reusage of the char[].

try {
this.result = new BigDecimal(str);
final int length = i - startPos;
if (reuse == null || reuse.length != length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change this to reuse.length < length to only create a new array in case more space is needed?

reuse[j] = (char) bytes[startPos + j];
}

this.result = new BigDecimal(reuse);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use the BigDecimal(char[] in, int offset, int len) and reuse the reuse object also if it is larger than then provided input.

@@ -120,7 +132,14 @@ public static final BigDecimal parseField(byte[] bytes, int startPos, int length
throw new NumberFormatException("There is leading or trailing whitespace in the numeric field.");
}

String str = new String(bytes, startPos, i);
return new BigDecimal(str);
final char[] reuse = new char[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

reuse is not reused here but created new in every method invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't notice it's a static method...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to rename the variable though.

@twalthr
Copy link
Contributor Author

twalthr commented Sep 20, 2016

Thanks again for the review @fhueske. I will fix your comments and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants