Skip to content

Conversation

@cyb70289
Copy link
Contributor

@cyb70289 cyb70289 commented Sep 1, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 1, 2021

@edponce
Copy link
Contributor

edponce commented Sep 1, 2021

LGTM. Thanks for resolving this issue.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks for doing this.


options.to_type = decimal256(76, 67);
CheckCastFails(ArrayFromJSON(int32(), "[0]"), options);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we file a follow up issue to allow truncation via an unsafe cast in these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do.

CheckDispatchBest(name, {int64(), decimal128(1, 0)},
{decimal128(1, 0), decimal128(1, 0)});
CheckDispatchBest(name, {decimal128(1, 0), int64()},
{decimal128(1, 0), decimal128(1, 0)});
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so an int64 is cast to a 1-digit decimal? Unrelated to this PR surely, but I'm a bit surprised :-)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is testing that given an integer and a decimal, the integer is cast to a decimal of the same width/precision/scale.

Copy link
Member

Choose a reason for hiding this comment

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

But surely the actual cast from int64 to decimal128(1,0) will fail in the precision check?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. Maybe we want the cast logic to be smarter about that, then. As you said, though, that can be tackled separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyb70289 cyb70289 closed this in bbecb6a Sep 2, 2021
@cyb70289 cyb70289 deleted the 13067-int2dec branch September 2, 2021 10:05
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
Closes apache#11045 from cyb70289/13067-int2dec

Authored-by: Yibo Cai <yibo.cai@arm.com>
Signed-off-by: Yibo Cai <yibo.cai@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants