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

Update REST Catalog impl for smoke tests and remove obsolete override for test #1

Merged

Conversation

amogh-jahagirdar
Copy link

… for test

Description

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Comment on lines -191 to 189
public void testDropTableWithMissingDataFile()
{
assertThatThrownBy(super::testDropTableWithMissingDataFile)
.hasMessageContaining("Table location should not exist");
}

@Test
Copy link
Author

Choose a reason for hiding this comment

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

This test was failing expectedly after the upgrade because we made the change to remove Puffin files as part of https://github.com/apache/iceberg/pull/9305/files . We used to expect this test to fail in this override because Puffin files would linger after the "DROP" and we have an assertion that validates that all files are removed so the test would fail. Now, this override is no longer needed, we expect the base testDropTableWithMissingDataFile to pass. So we can just remove the test override and rely on the base.

Comment on lines +50 to +51
static class TestingJdbcCatalog
extends JdbcCatalog implements ViewCatalog
Copy link
Author

Choose a reason for hiding this comment

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

This is needed since the upgraded 1.5 RestSessionCatalog will check if a view exists when performing a replace. This effectively means the underlying catalog being used must implement ViewCatalog otherwise the loadView call will fail since the response cannot be parsed.
To fix this, I'm implementing a TestingJdbcCatalog which implements ViewCatalog. We'll need this anyways in the REST View Catalog implementation PR. trinodb#19818

@ajantha-bhat ajantha-bhat merged this pull request into ajantha-bhat:bump Jan 12, 2024
1 check passed
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.

2 participants