-
-
Notifications
You must be signed in to change notification settings - Fork 800
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
BigInteger parsing in NumberInput can only support '1e10' format when fast parser mode is not used #986
Comments
@cowtowncoder apologies to be a pain but on further investigation, maybe we should rework the BigInteger support and not allow the parsing of any string that contains an 'e'/'E'. |
I am not sure I see why supporting e-format directly for Conversely I do agree that we should prevent parsing in case of |
The issue is that we inadvertently started supporting '1e10' format for BigInteger when I introduced a performance enhancement that allows numbers with strings of more than 1250 chars to be first parsed as BigDecimalParser (which has enhancements to speed up parsing of large strings). But BigDecimal allows '1e10' format. The recent issues we've had we |
@pjfanning I am also thinking that since limit of 1250 characters is used on using |
@pjfanning Understood. So how about just eliminating this code path now, before release? |
The change that led to this is #826 - my preference is not so much to remove this but that if we have more than 1250 chars, that we also then check for 'e' or 'E' in the string and fail if they appear - before parsing as BigDecimal. Is this ok? I have definitely found that using BigDecimal parsing for large strings is better than using So I'm not dead set against removing this code path altogether - just I have a preference to keep it but add the extra check to disallow 'e' notation for BigIntegers. |
@pjfanning I am ok with validation too if you think this optimization is still valid and potentially useful. We could also lower the threshold if there's evidence to support speed up occurs at shorter lengths too, but I am guessing you did some testing to reach this value (or maybe saw a reference suggesting cut-off point) |
If we go as far as removing the 1250 code path, do we also remove the |
@pjfanning No I think that check is needed for pretty much everything, where |
With #987, scientific notation ('1e10' format) is no longer supported. |
Users who need faster Biginteger parsing in jackson 21.5, you should consider #851 |
NumberInput.parseBigInteger uses BigDecimalParser when the string is large (over 1250 chars). BigDecimalParser can parse nums in '1e10' format and you can convert the BigDecimal to a BigInteger.
When the string size is less than 1250 chars,
new BigInteger
is used. This will fail for strings in '1e10' format. BigInteger does not support e notation directly.If you enable FastDoubleParser support, we use its JavaBigIntegerParser regardless of string size. This rejects '1e10' format in order to match the behaviour of
BigInteger
class.We should decide whether we want to ban '1e10' format for BigInteger in all cases (which is probably the case in jackson v2.14 and earlier) or if we want to press on and support '1e10' format in all cases. Might be better to ban the format - meaning we could remove the new StreamReadConstraints support for big int scale control.
The text was updated successfully, but these errors were encountered: