Fix createViewWithCustomMetadataLocation tests for cloudTest task by tmater · Pull Request #2405 · apache/polaris

tmater · 2025-08-20T08:44:16Z

This patch fixes the createViewWithCustomMetadataLocation tests for cloudTest tasks. The original test was generating temp directories internally, causing cloudTests to fail with BadRequestException instead of the expected ForbiddenException.

Changes:

Switched to Hadoop's Path (Java Path removes slashes, e.g. s3://bucket/path -> s3:/bucket/path)
Made base classes abstract to avoid running them
Implemented createViewWithCustomMetadataLocation to allow passing a custom location

Testing:

Verified locally

snazy · 2025-08-20T09:11:29Z

...in/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewAwsIntegrationTestBase.java


 /** Runs PolarisRestCatalogViewIntegrationTest on AWS. */
-public class PolarisRestCatalogViewAwsIntegrationTest
+public abstract class PolarisRestCatalogViewAwsIntegrationTestBase


Since we're renaming this type anyway, let's better use the protocol name, not the vendor name.

Suggested change

public abstract class PolarisRestCatalogViewAwsIntegrationTestBase

public abstract class PolarisRestCatalogViewS3IntegrationTestBase

Shall I change these 3 (non-view test bases) as well in one go:

PolarisRestCatalogAwsIntegrationTestBase

PolarisRestCatalogAzureIntegrationTestBase

PolarisRestCatalogGcpIntegrationTestBase

Also the 6 implementations:

RestCatalogAwsIT

RestCatalogAzureIT

RestCatalogGcpIT

RestCatalogViewAwsIT

RestCatalogViewAzureIT

RestCatalogViewGcpIT

I'd support it, yea

Mean, we support MinIO and a bunch of other S3 compatible systems, not only AWS. Sure, we could argue about GCS or ADLS. However, GCS and ADLS are specific, where a vendor name or "product suite" name are not.

Makes sense, renamed the classes and related parts.

snazy · 2025-08-20T09:12:44Z

...in/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewAwsIntegrationTestBase.java

 import java.util.List;
 import java.util.Optional;
-import java.util.stream.Stream;
+import org.apache.hadoop.fs.Path;


Hm, sure, this is just a test, but could we avoid Hadoop dependencies?

The idea behind Hadoop Path came because Iceberg does something "funky" with the metadataFileLocation() that resembles Hadoop Path behavior: It normalizes the scheme file:///... to file:/..., but it keeps s3://... as s3://....
For the assertion later I couldn't use java.nio.Path because that normalizes the scheme and creates an incorrect s3:/... path.

Just dug a bit more, I found a reference to Hadoop Path in Iceberg's LocationProviders:24, maybe this is how they do it as well, I could not find the exact location where it gets normalized.

I’m open to other approaches, but the only solution that came to mind was writing a custom assertion, which I’m also happy with.

Argh - the damn file URI scheme is so ambiguous (IMHO, because of multiple representations for the same thing, legacy representations, risk of interpreting those wrong).

If it's just about eliminating all host related parts, I'd say let's just use a regex to "fix" the paths?
Something like

protected static final Pattern FILE_LOCATION_PATTERN = Pattern.compile("file:/*(.*)"); protected String fixFileUri(String location) { var m = FILE_LOCATION_PATTERN.matcher(location); return m.matches() ? "file:/" + m.group(1) : location; }

WDYT? Would that work?

Just FYI In Polaris we have:

org.apache.polaris.core.storage.StorageLocation which standardizes file URIs to "file:///

but InMemoryStorageIntegration does the opposite 😅 :

polaris/polaris-core/src/main/java/org/apache/polaris/core/storage/InMemoryStorageIntegration.java

Line 74 in 532ee51

.map(str -> str.replace("file:///", "file:/"))

Hm - maybe let's ignore the file scheme specialty and just assume that it's "correct" if supplied.

Good point, I used java.nio.file.Path to remove the scheme.

...in/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewGcpIntegrationTestBase.java

.../java/org/apache/polaris/service/it/test/PolarisRestCatalogViewAzureIntegrationTestBase.java

This patch fixes the createViewWithCustomMetadataLocation tests for cloudTest tasks. The original test was generating temp directories internally, causing cloudTests to fail with BadRequestException instead of the expected ForbiddenException. Changes: - Switched to Hadoop's Path (Java Path removes slashes, e.g. s3://bucket/path -> s3:/bucket/path) - Made base classes abstract to avoid running them - Implemented createViewWithCustomMetadataLocation to allow passing a custom location Testing: - Verified locally

adutra · 2025-08-28T14:45:04Z

.../src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java

+  protected String getCustomMetadataLocationDir() {
+    return "";
+  }


Suggested change

protected String getCustomMetadataLocationDir() {

return "";

}

protected abstract String getCustomMetadataLocationDir();

adutra · 2025-08-28T14:45:46Z

.../src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java

  private static PolarisClient client;
  private static ManagementApi managementApi;
+  protected static final String POLARIS_IT_SUBDIR = "polaris_it";
+  protected static final String POLARIS_IT_CUSTOM_SUBDIR = "polaris_it_custom";


This constant seems unused.

adutra · 2025-08-28T14:51:57Z

...n/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewAdlsIntegrationTestBase.java

-  protected boolean shouldSkip() {
-    return Stream.of(BASE_LOCATION, TENANT_ID).anyMatch(Strings::isNullOrEmpty);
+  protected String getCustomMetadataLocationDir() {
+    return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);


Shouldn't this be:

Suggested change

return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);

return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_CUSTOM_SUBDIR, File.separator);

github-actions · 2025-09-30T02:01:39Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

* Update dependency com.fasterxml.jackson:jackson-bom to v2.20.1 (apache#2949) * Update dependency com.github.jk1:gradle-license-report to v3 (apache#2958) * Fix typos and spelling issues identified by codespell (apache#2959) * Fix findings with codespell * Fix findings with codespell * Fix regtest doc (apache#2955) * fix(deps): update dependency software.amazon.awssdk:bom to v2.37.3 (apache#2960) * Refactor Polaris cloudTests and adopt changes from PR apache#2405 (apache#2871) * Last merged commit 4a80c51 --------- Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Yong Zheng <yongzheng0809@gmail.com> Co-authored-by: Tamas Mate <50709850+tmater@users.noreply.github.com>

github-project-automation bot added this to Basic Kanban Board Aug 20, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 20, 2025

tmater marked this pull request as ready for review August 20, 2025 09:00

snazy reviewed Aug 20, 2025

View reviewed changes

tmater force-pushed the cloud_test_fix branch from 1c593b6 to 5a637e7 Compare August 24, 2025 18:21

tmater added 3 commits August 28, 2025 09:42

Addressing review comments, rebase

f17efa6

Self-review

c34120f

tmater force-pushed the cloud_test_fix branch from 59a0d79 to c34120f Compare August 28, 2025 09:20

adutra reviewed Aug 28, 2025

View reviewed changes

github-actions bot added the stale label Sep 30, 2025

github-actions bot closed this Oct 5, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Oct 5, 2025

tmater added a commit to tmater/polaris that referenced this pull request Oct 23, 2025

Refactor Polaris cloudTests and adopt changes from PR apache#2405

bb296dd

tmater mentioned this pull request Oct 23, 2025

Refactor Polaris cloudTests and adopt changes from PR #2405 #2871

Merged

snazy pushed a commit that referenced this pull request Nov 3, 2025

Refactor Polaris cloudTests and adopt changes from PR #2405 (#2871)

4a80c51

	public abstract class PolarisRestCatalogViewAwsIntegrationTestBase
	public abstract class PolarisRestCatalogViewS3IntegrationTestBase

	return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_SUBDIR, File.separator);
	return StorageUtil.concatFilePrefixes(BASE_LOCATION, POLARIS_IT_CUSTOM_SUBDIR, File.separator);

Conversation

tmater commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmater Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmater Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tmater Aug 24, 2025 •

edited

Loading

adutra Aug 20, 2025 •

edited

Loading

tmater Aug 24, 2025 •

edited

Loading