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

Introduce O(n^1.5) BigDecimal parser implementation #677

Merged

Conversation

ferenc-csaky
Copy link
Contributor

Covers the BigDecimal part of #577. The double/float implementations would require a much bigger refactor, since the example implementation follows a very different approach, but IMO it's worth to update only the BigDecimal without the others as well.

@cowtowncoder
Copy link
Member

@ferenc-csaky Sounds good -- thank you very much for the contribution! -- and it looks like I have CLA from you already.

Just one request: could this be rebased against 2.13? master is for Jackson 3.0 and I think this improvement would be great for 2.13 -- I can handle merging to master.

@cowtowncoder cowtowncoder added 2.13 performance Issue related to performance problems or enhancements labels Feb 4, 2021
@ferenc-csaky ferenc-csaky changed the base branch from master to 2.13 February 4, 2021 06:37
@ferenc-csaky
Copy link
Contributor Author

Changed the base branch to 2.13. My original plan was this as well, my dev. branch also originated from 2.13, but it seems I missed it in the PR. Corrected now, thanks!

@cowtowncoder
Copy link
Member

Just realized that none of the tests verifies new code path(s), I think. Will repurpose some from original... should have caught this earlier,.

cowtowncoder added a commit that referenced this pull request Feb 5, 2021
@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 5, 2021

@ferenc-csaky Hmmh. I think there may be a bug somewhere, some offset not being carried or used for calculations?
I added a test in 2.13 from the original changes and it looks like offset used for splits is 0 for some reason.
As far as I can see it gets passed correctly to BigDecimalParser (first suspected it might not but seems to).
Seems to only happen with character-backed parser, not byte-based (probably former will always have 0-based buffer for text value).

One thing that is tricky, too, is how to propagate underlying secondary fails, if those are possible (not sure if they are after fixing this).

@cowtowncoder
Copy link
Member

Looks like int numIdx = 0 in parseBigDecimal(int splitLen) is wrong: that should get set to off, somewhere. But simple attempts did not produce expected results.

@ferenc-csaky ferenc-csaky mentioned this pull request Feb 5, 2021
@ferenc-csaky
Copy link
Contributor Author

Oh yes, you are totally right, I didn't follow the source impl. exactly and I messed it up. Sorry about it. I just created a PR with the fix. Now the test you added runs correctly.

@cowtowncoder
Copy link
Member

@ferenc-csaky np. Just glad this was caught :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue related to performance problems or enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants