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-24958: Create new iceberg-catalog module #2138

Merged
merged 3 commits into from Apr 13, 2021

Conversation

marton-bod
Copy link
Contributor

@marton-bod marton-bod commented Mar 31, 2021

df80c15 - copy-pasting sources from iceberg-hive-metastore 0.11.x, this commit should not need a review
Please review: fff81a7 - Changes needed compared to Iceberg repo to make tests pass:

Added the same checkstyle files as for iceberg-handler.
Moved AssertHelpers to iceberg-catalog test sources.
Deleted TestHiveMetastore from iceberg-handler (since it's already in iceberg-catalog) but ported over the changes from the previous commit.

@marton-bod
Copy link
Contributor Author

@pvary @szlta @lcspinter can you please review? Thanks!

@marton-bod marton-bod changed the title Hive 24958 2 HIVE-24958: Create new iceberg-catalog module Mar 31, 2021
Copy link
Contributor

@lcspinter lcspinter left a comment

Choose a reason for hiding this comment

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

+1

@marton-bod
Copy link
Contributor Author

marton-bod commented Apr 9, 2021

Generally looks good to me.
A few nits only. Did we remove all the "hacks" we did for the iceberg-handler module which were needed until we have the metastore module?

Thanks for the review! Yes, the only hack we could remove with this one was this:
https://github.com/apache/hive/pull/2138/files#diff-9d31f72cca8ab08ae120d321c1b58816336936a23a4ef720ad65e79d3acbe743L357
(and also the removal of TestHiveMetastore from the handler module)
Most other hacks depend on Iceberg 0.12.0 coming out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants