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.

Sign up for GitHub

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

Jump to bottom

Core: Improvements around View catalog tests #8865

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Core: Improvements around View catalog tests #8865

merged 1 commit into from
Oct 20, 2023

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Oct 18, 2023

No description provided.

@github-actions github-actions bot added the core label Oct 18, 2023
@nastra nastra force-pushed the view-catalog-tests-improvements branch from e855121 to ee11184 Compare October 18, 2023 10:36
ajantha-bhat
ajantha-bhat approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@ajantha-bhat ajantha-bhat left a comment

Choose a reason for hiding this comment

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

As mentioned in #7913 (comment) and discussed offline, I too faced these problems when I extend the tests for Nessie Views testing.

I have now locally verified and confirm that these changes can help for catalogs to extend this class.

nastra
nastra commented Oct 18, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
@@ -56,10 +59,16 @@ public abstract class ViewCatalogTests<C extends ViewCatalog & SupportsNamespace

protected abstract Catalog tableCatalog();

@TempDir private Path tempDir;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously we were referring directly to the tmp dir (similar to CatalogTests), but it's better to use a randomly generated directory. I'll have a follow-up PR to also use a randomly generated temp dir for CatalogTests

Copy link
Member

Choose a reason for hiding this comment

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

Ack.

For me mkdir was failing when we directly use the directories outside tmp, for example updated_tmp.

org.apache.iceberg.exceptions.RuntimeIOException: Failed to create file: file:/updated_tmp/ns/view/metadata/00001-b53ce176-f85d-47ba-95db-f571fe7f95a0.gz.metadata.json
	at org.apache.iceberg.hadoop.HadoopOutputFile.createOrOverwrite(HadoopOutputFile.java:87)
	at org.apache.iceberg.view.ViewMetadataParser.internalWrite(ViewMetadataParser.java:181)
	at org.apache.iceberg.view.ViewMetadataParser.overwrite(ViewMetadataParser.java:161)
	at org.apache.iceberg.view.BaseViewOperations.writeNewMetadata(BaseViewOperations.java:139)
	at org.apache.iceberg.view.BaseViewOperations.writeNewMetadataIfRequired(BaseViewOperations.java:147)
	at org.apache.iceberg.nessie.NessieViewOperations.doCommit(NessieViewOperations.java:127)
	at org.apache.iceberg.view.BaseViewOperations.commit(BaseViewOperations.java:123)
	at org.apache.iceberg.view.SetViewLocation.lambda$commit$0(SetViewLocation.java:68)
	at org.apache.iceberg.util.Tasks$Builder.runTaskWithRetry(Tasks.java:413)
	at org.apache.iceberg.util.Tasks$Builder.runSingleThreaded(Tasks.java:219)
	at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:203)
	at org.apache.iceberg.util.Tasks$Builder.run(Tasks.java:196)
	at org.apache.iceberg.view.SetViewLocation.commit(SetViewLocation.java:66)
	at org.apache.iceberg.view.ViewCatalogTests.updateViewLocation(ViewCatalogTests.java:1480)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at org.projectnessie.junit.engine.MultiEnvTestEngine.execute(MultiEnvTestEngine.java:67)
Caused by: java.io.IOException: Mkdirs failed to create file:/updated_tmp/ns/view/metadata (exists=false, cwd=file:/Users/ajantha/Documents/workspace/icebergWorkspace/iceberg/nessie)
	at org.apache.hadoop.fs.ChecksumFileSystem.create(ChecksumFileSystem.java:455)
	at org.apache.hadoop.fs.ChecksumFileSystem.create(ChecksumFileSystem.java:440)
	at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:911)
	at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:892)
	at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:789)
	at org.apache.iceberg.hadoop.HadoopOutputFile.createOrOverwrite(HadoopOutputFile.java:85)
	... 18 more

So, I am neutral about changing it in CatalogTests as it only uses tmp.

nastra
nastra commented Oct 18, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
@@ -664,6 +684,41 @@ public void renameViewTargetAlreadyExistsAsTable() {
.hasMessageContaining("Cannot rename ns.view to ns.table. Table already exists");
}

@Test
public void renameTableTargetAlreadyExistsAsView() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was something that we identified during review of another PR

rdblue
rdblue reviewed Oct 18, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java Outdated
.hasMessageContaining("View does not exist: ns.view"),
throwable ->
assertThat(throwable)
.isInstanceOf(CommitFailedException.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems suspicious to me that both of these are allowed. Why shouldn't we standardize on NoSuchViewException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, I think it's better to standardize on NoSuchViewException in this case

rdblue
rdblue reviewed Oct 18, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java Outdated
@@ -225,8 +243,9 @@ public void createViewErrorCases() {
.withQuery(trino.dialect(), trino.sql())
.withQuery(trino.dialect(), trino.sql())
.create())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid view version: Cannot add multiple queries for dialect trino");
.isInstanceOf(Exception.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct. We should never allow ANY exception to be valid. Why is IllegalArgumentException not the correct one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one particularly fails with REST view catalog in CatalogHandlers here:

Expecting actual throwable to be an instance of:
  java.lang.IllegalArgumentException
but was:
  org.apache.iceberg.exceptions.BadRequestException: Malformed request: Invalid view version: Cannot add multiple queries for dialect trino

I'm open to better ideas on how we could solve this for this particular test case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this change for now. We can discuss it in the REST View support PR

rdblue
rdblue reviewed Oct 18, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
if (!overridesRequestedLocation()) {
assertThat(view.location()).isEqualTo(location);
} else {
assertThat(view.location()).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we allow overriding the location? Shouldn't the catalog fail if it chooses not to allow a custom location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this goes back to a similar approach that is used for CatalogTests (#4906)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I don't think we fail for tables with a REST catalog when the REST catalog overrides the given location

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. Thanks for looking into it!

@nastra nastra force-pushed the view-catalog-tests-improvements branch from 8faaa2c to a2c2745 Compare October 19, 2023 09:00
@rdblue rdblue merged commit 43fce1b into apache:main Oct 20, 2023
45 checks passed
@rdblue
Copy link
Contributor

rdblue commented Oct 20, 2023

Thanks, @nastra! Looks like this is also included in #7913 so it's good to get this in.

@nastra nastra deleted the view-catalog-tests-improvements branch October 21, 2023 07:25
@nastra nastra mentioned this pull request Oct 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajantha-bhat ajantha-bhat ajantha-bhat approved these changes

@rdblue rdblue rdblue approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nastra @rdblue @ajantha-bhat