Skip to content

Conversation

@pvary
Copy link
Contributor

@pvary pvary commented Oct 21, 2021

After #2565 the Catalogs.loadCatalog is synchronised with the CatalogUtil implementation.
Catalogs.hiveCatalog is not fixed, and also not tested.

This caused issues when I have tried to port this to the Hive repo.

The PR fixes the method, and also adds test.

@github-actions github-actions bot added the MR label Oct 21, 2021
@pvary
Copy link
Contributor Author

pvary commented Oct 21, 2021

CC: @jackye1995, @marton-bod

@pvary pvary added this to the Java 0.12.1 Release milestone Oct 21, 2021
@kbendick
Copy link
Contributor

Hi @pvary. On the mailing list, when you said:

This breaks Hive queries, if no catalog is set, but this still needs to be reviewed before merge.

Did you mean that this patch will break hive queries if not catalog is set after it's applied, or does it fix hive queries breaking if no catalog is set?

Thanks as always for your contributions 🙂

@pvary
Copy link
Contributor Author

pvary commented Oct 21, 2021

Did you mean that this patch will break hive queries if not catalog is set after it's applied, or does it fix hive queries breaking if no catalog is set?

@kbendick: My bad 😄
Ported #2565 to Hive, and the query tests started to break. In those tests we were not setting any catalogs, and the getCatalog returned a HiveCatalog but the Catalogs.hiveCatalog returned false. This is wrong, and also broke the tests, which started to fail when creating a table with Table location not set errors

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I checked it out locally, and some Flink checkstyle tests are failing, but that's certainly unrelated. I rebased locally off of master and the problems seem to go away. If you want to rebase and push, go ahead, but I don

@rdblue
Copy link
Contributor

rdblue commented Oct 26, 2021

Looks good to me. Thanks, @pvary! Sorry it took me a while to get to this review.

@rdblue rdblue merged commit d3a6107 into apache:master Oct 26, 2021
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 27, 2021
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 31, 2021
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Nov 1, 2021
@pvary
Copy link
Contributor Author

pvary commented Nov 2, 2021

Thanks @kbendick and @rdblue for the review and merge!

@pvary pvary deleted the hivacatfix branch November 2, 2021 08:55
izchen pushed a commit to izchen/iceberg that referenced this pull request Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants