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.

Sign up for GitHub

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

Jump to bottom

HIVE-26658: INT64 Parquet timestamps cannot be mapped to most Hive numeric types #3698

Closed
wants to merge 1 commit into from
Closed

HIVE-26658: INT64 Parquet timestamps cannot be mapped to most Hive numeric types #3698

wants to merge 1 commit into from

Conversation

zabetak
Copy link
Contributor

@zabetak zabetak commented Oct 24, 2022

What changes were proposed in this pull request?

  1. Unify converters from Parquet INT64 to Hive types.
  2. Add tests reading from Parquet INT64 timestamp to various Hive numeric types.

Why are the changes needed?

Restore backward compatibility; allow mapping INT64 timestamps with timestamp annotation to the following Hive numeric types:

  • TINYINT
  • SMALLINT
  • INT
  • DOUBLE
  • FLOAT
  • DECIMAL

For more details see HIVE-26658.

Does this PR introduce any user-facing change?

Avoids errors/exceptions when attempting to map Parquet INT64 with timestamp to anything except TIMESTAMP & BIGINT.

How was this patch tested?

mvn test -Dtest=TestETypeConverter
mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile=parquet_int64_timestamp_to_numeric.q

@sonarcloud
Copy link

sonarcloud bot commented Oct 24, 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 0 Code Smells

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

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.

@github-actions github-actions bot added the stale label Dec 24, 2022
@zabetak
Copy link
Contributor Author

zabetak commented Dec 24, 2022

Still relevant, please keep this open!

@github-actions github-actions bot removed the stale label Dec 25, 2022
cnauroth
cnauroth reviewed Dec 27, 2022
View reviewed changes
Copy link
Contributor

@cnauroth cnauroth left a comment

Choose a reason for hiding this comment

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

Hello @zabetak . This looks good to me, though the CI run from a few months ago showed a test failure. Is it because the .q and .q.out files have a different ordering for the ALTER TABLE statements?

@zabetak
Copy link
Contributor Author

zabetak commented Jan 2, 2023

Thanks for the review @cnauroth ! Indeed I changed the order of the statements in the .q file just before opening the PR and forgot to update the respective .q.out.

I rebased the PR against latest master and updated the stale .q.out file so I am hoping that now all tests will come back green.

@sonarcloud
Copy link

sonarcloud bot commented Jan 2, 2023

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 0 Code Smells

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

@zabetak
Copy link
Contributor Author

zabetak commented Jan 3, 2023

Hey @scarlin-cloudera , since this is a follow-up of a ticket that you worked on can you please have a look as well?

cnauroth
cnauroth approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@cnauroth cnauroth left a comment

Choose a reason for hiding this comment

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

The tests passed on the last run, so +1 (non-binding). Thanks for the update, @zabetak !

@scarlin-cloudera
Copy link
Contributor

LGTM

ayushtkn
ayushtkn approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

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

LGTM

@zabetak zabetak closed this in 5519bb3 Jan 10, 2023
@zabetak zabetak deleted the HIVE-26658 branch January 10, 2023 11:47
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
@zabetak @yeahyung
HIVE-26658: INT64 Parquet timestamps cannot be mapped to most Hive nu…
d18d88c 
…meric types (Stamatis Zampetakis reviewed by Chris Nauroth, Steve Carlin, Ayush Saxena)

Closes apache#3698
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
@zabetak @tarak271
HIVE-26658: INT64 Parquet timestamps cannot be mapped to most Hive nu…
0c2eb38 
…meric types (Stamatis Zampetakis reviewed by Chris Nauroth, Steve Carlin, Ayush Saxena)

Closes apache#3698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnauroth cnauroth cnauroth approved these changes

@ayushtkn ayushtkn ayushtkn approved these changes

Assignees
No one assigned
Labels
tests passed
Projects
None yet
Milestone
No milestone
5 participants
@zabetak @scarlin-cloudera @cnauroth @ayushtkn @kgyrtkirk