liamzwbao commented on code in PR #1126:
URL: https://github.com/apache/polaris/pull/1126#discussion_r2013094692
##########
quarkus/service/src/intTest/java/org/apache/polaris/service/quarkus/it/QuarkusRestCatalogViewFileIT.java:
##########
@@ -20,20 +20,37 @@
import io.quarkus.test.junit.QuarkusIntegrationTest;
import java.lang.reflect.Field;
+import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.Paths;
import org.apache.iceberg.view.ViewCatalogTests;
import
org.apache.polaris.service.it.test.PolarisRestCatalogViewFileIntegrationTest;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.AnnotatedElementContext;
+import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.api.io.TempDirFactory;
@QuarkusIntegrationTest
public class QuarkusRestCatalogViewFileIT extends
PolarisRestCatalogViewFileIntegrationTest {
@BeforeEach
- public void setUpTempDir(@TempDir Path tempDir) throws Exception {
+ public void setUpTempDir(@TempDir(factory = CustomTempDirFactory.class) Path
tempDir)
+ throws Exception {
// see https://github.com/quarkusio/quarkus/issues/13261
Field field = ViewCatalogTests.class.getDeclaredField("tempDir");
field.setAccessible(true);
field.set(this, tempDir);
}
+
+ private static class CustomTempDirFactory implements TempDirFactory {
+ @Override
+ public Path createTempDirectory(
+ AnnotatedElementContext elementContext, ExtensionContext
extensionContext)
+ throws Exception {
+ Path basePath = Paths.get(BASE_LOCATION.replaceFirst("file://", ""));
Review Comment:
Hi @gh-yzou, I did some further investigation on this. I found that even if
we apply a similar fix like in #193, the test from the superclass will still
fail. This is because `write.metadata.path` is only added to `allowedLocations`
for the root namespace — this behavior is by design in Polaris per
[link](https://github.com/apache/polaris/pull/193/files#diff-607cdd8c6ea78443988359e42ffec3091a4c84bc9151575772ee3b90164e03f3R184-R191).
What I mean is that we can specify any location like `file://tmp/`, and it
will be added to `allowedLocations` **only if** we include it when creating the
namespace. So, to make the test pass, we’d need something like the following:
```java
TableIdentifier identifier = TableIdentifier.of("ns", "view");
String location = Paths.get("file://tmp/").toString();
String customLocation = Paths.get("file://tmp/",
"custom-location").toString();
if (requiresNamespaceCreate()) {
catalog()
.createNamespace(
identifier.namespace(),
// Set the custom location in the root namespace
ImmutableMap.of(ViewProperties.WRITE_METADATA_LOCATION,
customLocation));
}
assertThat(catalog().viewExists(identifier)).as("View should not
exist").isFalse();
View view =
catalog()
.buildView(identifier)
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withDefaultCatalog(catalog().name())
.withQuery("spark", "select * from ns.tbl")
.withProperty(ViewProperties.WRITE_METADATA_LOCATION, customLocation)
.withLocation(location)
.create();
assertThat(view).isNotNull();
assertThat(catalog().viewExists(identifier)).as("View should
exist").isTrue();
assertThat(view.properties()).containsEntry("write.metadata.path",
customLocation);
assertThat(((BaseView) view).operations().current().metadataFileLocation())
.isNotNull()
.startsWith(customLocation);
```
So my conclusion is that the expected behavior of this test is not
applicable to Polaris. I think we can address this in two ways:
1. Use `TempDirFactory` to **change the temp directory** in the superclass
test so that we can always follow upstream changes. Then, **write another**
Polaris-specific test using the above code in another PR with the fix for
createView flow.
2. **Skip** this test in this PR, and use a follow-up PR to **override** it
with Polaris-specific behavior using the code above. This is simpler and more
straightforward, but we won’t catch future changes in the superclass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
