velox
velox copied to clipboard
facebookincubator
Support decimal operation precision not loss mode
Each of the decimal operation functions is registered as two functions such as add_deny_precision_loss and add.
When allowing precision loss, establishing the result type of an arithmetic operation happens according to Hive behavior and SQL ANSI 2011 specification, i.e. rounding the decimal part of the result if an exact representation is not possible. Otherwise, NULL is returned in those cases, as previously.
When not allowing precision loss, not rounding the decimal part.
For example,
| adding decimal(38, 7) and decimal(10, 0) result type | adding 1.1232154 and 1 | |
|---|---|---|
| allow precision loss | decimal(38, 6) | 2.123215 |
| deny precision loss | decimal(38, 7) | 2.1232154 |
Spark implementation: https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala#L814
Deploy Preview for meta-velox canceled.
| Name | Link |
|---|---|
| Latest commit | 412e0041377b01950b74b8f9d0fd2313169b176e |
| Latest deploy log | https://app.netlify.com/sites/meta-velox/deploys/67315cd274896d0008840ec8 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
As this discussion https://github.com/facebookincubator/velox/issues/10467, we cannot set it at runtime. @rui-mo
Talked with @jinchengchenghh offline, and she will help update this PR to support session-level precision loss mode.
Addressed all the comments, could you help review again? Thanks! @rui-mo
Yes, thanks for your kindly review. I forgot to update the PR description when the implement was changed, now it is registered as different names as we discussed in issue https://github.com/facebookincubator/velox/issues/10467#issuecomment-2339486865 Updated and added the example in PR description. @mbasmanova
@jinchengchenghh
Otherwise, NULL is returned in those cases, as previously.
What does this refer to? Isn't current before for an operation to fail? Would you provide an example where it returns NULL?
When not allowing precision loss, not rounding the decimal part.
It might be helpful to clarify the tradeoff of using allowing vs. not allowing precision loss. Otherwise, one may wonder why would we ever what to allow precision loss.
I assume some operations success if precision loss is allowed and fail if not. Would be nice to add such examples to PR description and docs.
allow-precision-loss name might be confusing because 'precision' in 'allow-precision-loss' is not the same as 'precision' in decimal type. It might be helpful to clarify some more to avoid confusion.
@jinchengchenghh
Otherwise, NULL is returned in those cases, as previously.
What does this refer to? Isn't current before for an operation to fail? Would you provide an example where it returns NULL?
Yes, overflow may result in NULL. This description origins from Spark config description. I agree it is more likely return null for some edge cases when not allowing precision loss, I tried before to calculate the edge case, but I could not find an example, I will scan Spark test cases to try again. https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala#L3373
Added the example and update document, could you help review again? Thanks! @mbasmanova
Updated the document, could you help review again? Thanks! @mbasmanova
@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@jinchengchenghh could you rebase, and update the PR?
There are a lot of broken builds, but I know there was a bad merge yesterday, so it's probably due to that.
@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@kevinwilfong merged this pull request in facebookincubator/velox@3884939853312782e5252084f28cd3f806d705fe.
Conbench analyzed the 1 benchmark run on commit 38849398.
There were no benchmark performance regressions. 🎉
The full Conbench report has more details.
![conbench-facebook[bot] avatar](https://avatars.githubusercontent.com/in/241235?v=4 "Published by a pub.dev verified publisher"){kind=small}