Remove exceptions from decimal validator message #613
Conversation
| decimal_overflow_validator_.validate(data[i]); | ||
| min = std::min(min, data[i]); | ||
| max = std::max(max, data[i]); | ||
| if (decimal_overflow_validator_.validate(data[i])) { |
There was a problem hiding this comment.
Do we just ignore out-of-range decimal values now or do we assume it's checked on the arrow decimal conversion? Shouldn't this validation always succeed?
There was a problem hiding this comment.
Well, I am skeptical we ever hit this branch but can't guarantee it. So, we ignore out of range values. I suppose this could pose a problem later, and maybe we should just remove this code altogether and make sure we validate the decimal range on import. Alternatively we could try aborting with an error message (or perhaps an exception) and not specify whether the decimal is above or below the range. It is silly to pay the cost of a function call for every single value in every single integer column, even if that function just returns.
There was a problem hiding this comment.
On one side, making a range check combined with stats computation is very reasonable and reduces the number of scans. On the other side, our only external data source is Arrow, which uses another decimal format and always requires conversion. So, we can probably make the converter responsible for the check and remove the decimal validator from encoders.
alexbaden
force-pushed
the
alex/remove_decimal_validator
branch
from
30ff9a1 to
e40373e
Compare
|
As expected this validation also happens during arrow parse: so as long as the arrow and HDK types match, decimal validation is actually completely unnecessary. I have updated the PR to remove it entirely. |
alexbaden
force-pushed
the
alex/remove_decimal_validator
branch
from
e40373e to
ef013f2
Compare
Decimal validation occurs on parse, when the input value (string, etc) is converted to an arrow::DecimalXX value.
Using exceptions results in substantial performance loss in tight loops, where this code is often used.