Skip to content

Conversation

@pravindra
Copy link
Contributor

No description provided.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. LGTM.

int i = 0;
for (auto& in_arg : function->args()) {
if (i == 3) {
auto y_neg_value = ir_builder()->CreateNeg(&in_arg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if this could overflow, but the decimal type is limited to 38 decimal digits, which is too small for 2**127.

As a side note, it seems we don't validate inputs to the Decimal128 type and happily accept larger numbers:

>>> ty = pa.decimal128(39, 0)                                                                            
>>> arr = pa.array([(2**127) - 1], type=ty)                                                              
>>> arr                                                                                                  
<pyarrow.lib.Decimal128Array object at 0x7f9b89444e08>
[
  170141183460469231731687303715884105727
]
>>> arr = pa.array([(2**127)], type=ty)                                                                  
>>> arr                                                                                                  
<pyarrow.lib.Decimal128Array object at 0x7f9b89444138>
[
  -170141183460469231731687303715884105728
]
>>> arr = pa.array([-(2**127)], type=ty)                                                                 
>>> arr                                                                                                  
<pyarrow.lib.Decimal128Array object at 0x7f9b89432688>
[
  -170141183460469231731687303715884105728
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a jira to ensure gandiva fails when the type has a precision/scale is > 38.

For the actual values itself, I can add a IsValid() api to BasicDecimal (that checks against a min/max value). but, checking this each time will be a perf overhead. maybe, we should have a conf variable to do overflow checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, I'd say people who use decimals value correctness. Though it's clear our C++ API currently doesn't check...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created ARROW-4569 and ARROW-4570 to follow up on these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants