Skip to content

Core: Avoid creating new metadata file when registerTable API is used - #6591

Merged
szehon-ho merged 3 commits into
apache:masterfrom
krvikash:avoid-creating-new-metadata-file-in-register-table
Jan 25, 2023
Merged

Core: Avoid creating new metadata file when registerTable API is used#6591
szehon-ho merged 3 commits into
apache:masterfrom
krvikash:avoid-creating-new-metadata-file-in-register-table

Conversation

@krvikash

@krvikashkrvikash commented Jan 14, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

HiveTableOperations and NessieTableOperations were already handling this case, but other table operations are missing this case.

Follow up of #6512 (comment)

@krvikashkrvikash changed the title Avoid creating new metadata file when registerTable API is usedAws, Core, Dell, Hive, Nessie : Avoid creating new metadata file when registerTable API is usedJan 14, 2023
@krvikashkrvikash changed the title Aws, Core, Dell, Hive, Nessie : Avoid creating new metadata file when registerTable API is usedAws, Core, Dell, Hive, Nessie: Avoid creating new metadata file when registerTable API is usedJan 14, 2023
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch from c8cb001 to f597ce3CompareJanuary 14, 2023 23:08
ajantha-bhat
ajantha-bhat approved these changes Jan 17, 2023
View reviewed changes

@ajantha-bhatajantha-bhat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

PR description can be simplified by just keeping Core: instead of all the modules.

@krvikashkrvikash changed the title Aws, Core, Dell, Hive, Nessie: Avoid creating new metadata file when registerTable API is usedCore: Avoid creating new metadata file when registerTable API is usedJan 17, 2023
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch from f597ce3 to cda5a05CompareJanuary 17, 2023 16:46
szehon-ho
szehon-ho reviewed Jan 20, 2023
View reviewed changes

@szehon-hoszehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense to me, left some style comments

Comment threadcore/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated
Comment threadcore/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java Outdated
@krvikash
Avoid creating new metadata file when registerTable API is used
85aaf12
HiveTableOperations and NessieTableOperations were already handling this case,
but other table operations are missing this case.
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch from cda5a05 to 85aaf12CompareJanuary 21, 2023 20:24
szehon-ho
szehon-ho reviewed Jan 23, 2023
View reviewed changes

@szehon-hoszehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the comments. One more small one, and if it is not too much trouble, could we add a unit test in TestHiveCatalog for it for registerTable?

Comment threadhive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
Comment threadcore/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java Outdated
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch from cc1e34d to ccc8666CompareJanuary 23, 2023 19:16
@szehon-ho

szehon-ho commented Jan 24, 2023

Copy link
Copy Markdown
Member

Thanks @krvikash looks good to me, just wanted to see if we can add a test in TestHiveCatalog? (Ignore if you read already and still working on it)

@krvikash

krvikash commented Jan 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks, @szehon-ho for reviewing. Sorry, I missed adding a test case in TestHiveCatalog. I have added it now.

szehon-ho
szehon-ho reviewed Jan 25, 2023
View reviewed changes

@szehon-hoszehon-ho left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding test, one comment about that

Comment threadhive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch 2 times, most recently from 5dca53e to b146e87CompareJanuary 25, 2023 06:28
@krvikash
krvikashforce-pushed the avoid-creating-new-metadata-file-in-register-table branch from b146e87 to 05cccd6CompareJanuary 25, 2023 06:28
@szehon-ho

szehon-ho commented Jan 25, 2023

Copy link
Copy Markdown
Member

Thanks for last changes, Ill commit this tomorrow unless there's more comments.

@krvikash

krvikash commented Jan 25, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks, @szehon-ho | @ajantha-bhat for reviewing the PR.

@szehon-ho
szehon-ho merged commit 81bf8d3 into apache:masterJan 25, 2023
@szehon-ho

szehon-ho commented Jan 25, 2023

Copy link
Copy Markdown
Member

Merged, thanks @krvikash for change, @ajantha-bhat for review

@krvikash
krvikash deleted the avoid-creating-new-metadata-file-in-register-table branch January 25, 2023 19:28
@ajantha-bhatajantha-bhat mentioned this pull request Feb 4, 2023
Closed
krvikash added a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023
@krvikash
Core: Avoid creating new metadata file on registerTable (apache#6591)
29e74c9
@krvikashkrvikash mentioned this pull request Jun 14, 2023
Merged
@InvisibleProgrammerInvisibleProgrammer mentioned this pull request Jun 20, 2023
Merged
@dramaticllydramaticlly mentioned this pull request Feb 26, 2025
Closed
@hantangwangdhantangwangd mentioned this pull request Sep 15, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szehon-hoszehon-hoszehon-ho approved these changes
+1 more reviewer
@ajantha-bhatajantha-bhatajantha-bhat approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AWScoreDELLhiveNESSIE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@krvikash@szehon-ho@ajantha-bhat