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: Fix writing of Date, Decimal, Time and UUID types. #2126

Merged
merged 1 commit into from
Jan 25, 2021
Merged

Hive: Fix writing of Date, Decimal, Time and UUID types. #2126

merged 1 commit into from
Jan 25, 2021

Conversation

lcspinter
Copy link
Contributor

No description provided.

@lcspinter
Copy link
Contributor Author

@marton-bod @pvary If you have time, could you please review this change? Thanks

marton-bod
marton-bod reviewed Jan 20, 2021
View reviewed changes
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerWithEngine.java Outdated
Types.TimestampType.withoutZone(), Types.StringType.get(), Types.BinaryType.get(),
Types.DecimalType.of(3, 1), Types.UUIDType.get(), Types.FixedType.ofLength(5),
Types.DecimalType.of(3, 1), Types.UUIDType.get(), Types.FixedType.ofLength(11),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we write different decimal scales/precisions? If so, can we add a second one here, that has a different scale to test?

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 would rather not add a new entry to this list. If we would like to test the many flavours of the Decimal type, I guess we should do it in a dedicated method/class.

pvary
pvary approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@pvary pvary left a comment

Choose a reason for hiding this comment

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

LGTM +1 (one small question left over, but not blocker)

@lcspinter
Copy link
Contributor Author

Thank @pvary @marton-bod for the review!

@lcspinter lcspinter force-pushed the CDPD-21432 branch 2 times, most recently from 73990fe to 2ccde3d Compare January 25, 2021 12:14
@lcspinter lcspinter changed the title Hive: Add write test for all supported types. Hive: Fix writing of Date, Decimal, Time and UUID types. Jan 25, 2021
@pvary pvary merged commit 5ed15d0 into apache:master Jan 25, 2021
@lcspinter lcspinter deleted the CDPD-21432 branch January 25, 2021 14:33
@pvary
Copy link
Contributor

pvary commented Jan 25, 2021

Thanks for the fix @lcspinter and @marton-bod for the review!
Please do not forget to add different scale/precision tests for the Decimal later.

@rdblue
Copy link
Contributor

rdblue commented Jan 25, 2021

Thanks for reviewing, @pvary! Glad to see you can commit now!

@pvary
Copy link
Contributor

pvary commented Jan 26, 2021

Thanks for reviewing, @pvary! Glad to see you can commit now!

Thanks for helping through the first steps! 😄

@pvary pvary added this to the Java 0.11.1 Release milestone Feb 18, 2021
aokolnychyi pushed a commit that referenced this pull request Mar 24, 2021
@lcspinter @aokolnychyi
Hive: Fix writing of Date, Decimal, Time and UUID types. (#2126)
e12d468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marton-bod marton-bod marton-bod approved these changes

@pvary pvary pvary approved these changes

Assignees
No one assigned
Labels
hive MR
Projects
None yet
Milestone
Java 0.11.1 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@lcspinter @pvary @rdblue @marton-bod