ARROW-17995: [C++] Fix json decimals not being rescaled based on the explicit schema #14380
Conversation
|
|
There was a problem hiding this comment.
Thanks for this @stiga-huang . I think we should be stricter and not let precision loss happen silently.
cpp/src/arrow/json/converter_test.cc
Outdated
| {"" : "02.0000000000"} | ||
| {"" : "30.0000000000"} | ||
| {"" : "30.01"} | ||
| {"" : "30.0000000000123"} |
There was a problem hiding this comment.
Do we want truncation to happen silently or would we rather get an error here?
There was a problem hiding this comment.
For example, our CSV reader would emit an error instead of dropping some digits like this.
There was a problem hiding this comment.
Yeah, I think users would like different behaviors. E.g. in Impala, we'd like Arrow to continue reading the remaining rows instead of throwing an error and stop.
I think we can add an option to choose the behavior.
There was a problem hiding this comment.
I realized that the error handling in other converters are also stop and return errors, e.g.
arrow/cpp/src/arrow/json/converter.cc
Lines 132 to 134 in d67a210
So I tend to the same behavior now. I think we can add the option for configurable error handling behavior in a new JIRA.
There was a problem hiding this comment.
Thanks. Feel free to ping once that is done. Hopefully we can get fixed in time for 10.0.0.
pitrou
force-pushed
the
ARROW-17995-rescale-json-decimal
branch
from
907c69f to
eef9f58
Compare
|
Thanks for your review! @pitrou |
…explicit schema (apache#14380) arrow::json::DecimalConverter::Convert() currently read the decimal values using the parsed precision and scale. This produces wrong results if the parsed scale doesn't match the output scale (specified by explicit schema). More details on how to reproduce the issue are in the JIRA description. This patch fixes json::DecimalConverter::Convert() to rescale the values based on the output scale. Unit tests are added as well. Lead-authored-by: stiga-huang <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
|
Benchmark runs are scheduled for baseline = aeba616 and contender = 289e0c9. 289e0c9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…explicit schema (apache#14380) arrow::json::DecimalConverter::Convert() currently read the decimal values using the parsed precision and scale. This produces wrong results if the parsed scale doesn't match the output scale (specified by explicit schema). More details on how to reproduce the issue are in the JIRA description. This patch fixes json::DecimalConverter::Convert() to rescale the values based on the output scale. Unit tests are added as well. Lead-authored-by: stiga-huang <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
arrow::json::DecimalConverter::Convert() currently read the decimal values using the parsed precision and scale. This produces wrong results if the parsed scale doesn't match the output scale (specified by explicit schema).
More details on how to reproduce the issue are in the JIRA description. This patch fixes json::DecimalConverter::Convert() to rescale the values based on the output scale. Unit tests are added as well.