Skip to content

HIVE-29019: Test Hive Iceberg REST Catalog with the compatibility kit - #5887

Merged
okumin merged 8 commits into
apache:masterfrom
okumin:HIVE-29019-iceberg-rck
Jun 23, 2025
Merged

HIVE-29019: Test Hive Iceberg REST Catalog with the compatibility kit#5887
okumin merged 8 commits into
apache:masterfrom
okumin:HIVE-29019-iceberg-rck

Conversation

@okumin

@okumin okumin commented Jun 21, 2025

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Test the Iceberg REST API with CatalogTests with RESTCatalog.

https://issues.apache.org/jira/browse/HIVE-29019

Why are the changes needed?

For better test coverage.

Does this PR introduce any user-facing change?

No. This feature has not been shipped.

How was this patch tested?

mvn clean test -pl metastore-rest-catalog

private RESTResponse dropNamespace(Map<String, String> vars) {
if (asNamespaceCatalog != null) {
CatalogHandlers.dropNamespace(asNamespaceCatalog, namespaceFromPathVars(vars));
return null;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a bug.

@deniskuzZ deniskuzZ Jun 22, 2025

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.

:) nice

@okumin
okumin force-pushed the HIVE-29019-iceberg-rck branch from 605fc5e to afdf7ac Compare June 22, 2025 03:32
@okumin okumin changed the title [WIP] HIVE-29019: Test Hive Iceberg REST Catalog with the compatibility kit HIVE-29019: Test Hive Iceberg REST Catalog with the compatibility kit Jun 22, 2025
@okumin
okumin marked this pull request as ready for review June 22, 2025 03:33
class TestRESTCatalogTestsAuthJwt extends BaseRESTCatalogTests {
@RegisterExtension
private static final HiveRESTCatalogServerExtension REST_CATALOG_EXTENSION = HiveRESTCatalogServerExtension.builder()
.jwt().build();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We may potentially parameterize test classes. I'm currently separating classes because each needs a different setup of HMS, and each has different patterns of authentication errors. I'm not confident and open to this point.

@okumin

okumin commented Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

The overview after we add View support will be as follows.
okumin@70953a4

@deniskuzZ deniskuzZ 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 @okumin, looks awesome, just a few minor comments.
I guess we should merge it before the No Auth PR?

@deniskuzZ

deniskuzZ commented Jun 22, 2025

Copy link
Copy Markdown
Member

btw, i've noticed that we are missing a few APIs in HMSCatalogAdapter:

  1. NAMESPACE_EXISTS,
  2. TABLE_EXISTS,
  3. VIEW_EXISTS

oh, I see you've already fixed that in #5887 (comment) 👍

@okumin

okumin commented Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

I guess we should merge it before the No Auth PR?

I think so. Regression tests will unblock many feature developments, including caching.

btw, i've noticed that we are missing a few APIs in HMSCatalogAdapter:

There is a small trick here. When /v1/config returns an empty list as available endpoints, the REST client does not use the three endpoints. That's why our implementation passes all the tests. I will update /v1/config and implement the three when we implement view-related endpoints.

@deniskuzZ deniskuzZ 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.

+1, pending tests

@sonarqubecloud

Copy link
Copy Markdown

@okumin

okumin commented Jun 23, 2025

Copy link
Copy Markdown
Contributor Author

CI is green. @deniskuzZ Thanks for reviewing this pull request!

@okumin
okumin merged commit 459b1e5 into apache:master Jun 23, 2025
@okumin
okumin deleted the HIVE-29019-iceberg-rck branch June 23, 2025 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants