Use a Parsers.jl-based parser implementation
#80
Conversation
Simplify Parsers integration
src/parse.jl
Outdated
| # all digits are zero | ||
| i = zero(T) | ||
| elseif backing_integer_digits < 0 | ||
| # All digits are past our precision, no overflow possible |
There was a problem hiding this comment.
Maybe I'm not understanding the comment quite right, but this makes it sound like it's backwards; I was expecting a phrase like "if any digits are past our precision, overflow is possible" or the inverse of "No digits are past our precision, so no overflow possible"
There was a problem hiding this comment.
Overflow is not possible here but inexact error is possible if we're not okay with rounding. In my mind overflow is when we try to parse "130" FD{Int8,0} and inexact would happen if we tried to parse "100.1" as FD{Int8,0}
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com> Co-authored-by: Nathan Daly <nathan.daly@relational.ai>
Co-authored-by: Nick Robinson <npr251@gmail.com>
| function _divpow10!(x::T, code, pow, mode::RoundingMode) where {T} | ||
| return div(x, _shift(one(T), pow), mode), code | ||
| end |
There was a problem hiding this comment.
Not something for right now, but just sharing this because it's so super freaking cool:
#45
Todd came up with this approach to improve the performance of dividing by power of ten. Basically the idea is that you can skip the div, which is a very expensive operation, by instead multiplying by (2^64 / 10 / 2^64), and since you can precompute 2^64 / 10 as a constant, and then / 2^64 can be done by just a bit-shift, you can skip the divide. Really clever!
But I never managed to merge that PR, so............................. :)
I think we just leave that optimization aside here too, but it's fun and something cool to think about for the future! :)
There was a problem hiding this comment.
I'm sorry that I wasn't able to review today. I was pulled into an oncall investigation of a customer incident today, and it unexpectedly took up a bunch of my day. :[
I'm going to merge though, based on Jacob's review, and based on the preliminary review we did together over zoom! :) It does look like a much better approach.
Thanks all!
This hooks into the floating point parsing machinery from
Parsers.jl, where we also accumulate all the digits and note the effective exponent before we do "scaling" -- forFixedDecimals, the scaling means padding the backing integer with zeros or rounding them as necessary. This change should improve performance noticeably as we no longer allocate any strings during parsing.RoundNearestrounding was redone during parsing to match the rounding behaviors on already parsed integers.cc: @quinnj @NHDaly