Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HIVE-26573: Fix ClassCastException issues for Decimal64 vectorization. #3630

Merged
merged 4 commits into from
Oct 21, 2022

Conversation

scarlin-cloudera
Copy link
Contributor

A ClassCastException error was being thrown while deserializing a Decimal64Vector.

It is unnecessary for the code to check the DataTypePhysicalVariation property within VectorDeserializeRow. There is already enough information to determine that the binding should bedone with the Decimal64Vector. So the code now just checks for the instance when deciding on the binding.

Another bug was found while implementing this fix. The Lazy reads were always converting the decimal into its decimal64 form which isn't necessary when there is no decimal64 binding. This code is now pushed into VectorDeserializeRow which is the only place in the repository which references this variable.

What changes were proposed in this pull request?

See commit description

Why are the changes needed?

See commit description

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit test added

A ClassCastException error was being thrown while deserializing a Decimal64Vector.

It is unnecessary for the code to check the DataTypePhysicalVariation property within
VectorDeserializeRow.  There is already enough information to determine that the binding
should bedone with the Decimal64Vector. So the code now just checks for the instance
when deciding on the binding.

Another bug was found while implementing this fix. The Lazy reads were always converting
the decimal into its decimal64 form which isn't necessary when there is no decimal64
binding. This code is now pushed into VectorDeserializeRow which is the only place in
the repository which references this variable.
@pudidic
Copy link
Contributor

pudidic commented Oct 5, 2022

+1 Looks good to me.

But there's a failing test on Jenkins. It ran successfully on my laptop multiple times. Could you push it with a new empty commit to re-trigger the CI? You can use git commit --allow-empty option for it.

@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@pudidic pudidic left a comment

Choose a reason for hiding this comment

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

+1. Looks good to me. As it's a trivial change, I'll merge it.

@pudidic pudidic merged commit d3c35ff into apache:master Oct 21, 2022
okumin pushed a commit to zookage/hive that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants