Fix matching file prefixes for FILE based catalogs #505
Conversation
Ensure matching File prefixes when using polaris instance with FILE based storage.
adithyashanker22
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
adithyashanker22
changed the title
| @@ -93,6 +93,9 @@ public boolean isChildOf(StorageLocation potentialParent) { | |||
| } else { | |||
| String slashTerminatedLocation = ensureTrailingSlash(this.location); | |||
| String slashTerminatedParentLocation = ensureTrailingSlash(potentialParent.location); | |||
| if (slashTerminatedLocation.startsWith("/") && slashTerminatedParentLocation.startsWith("file:///")) { | |||
| slashTerminatedLocation = "file://" + slashTerminatedLocation; | |||
There was a problem hiding this comment.
I recommend standardizing the URI schema to file:/// instead of file://. The file:/// format is the correct URI schema, whereas file://path is not. To make this consistent and maintainable, we could define a constant like this:
public static final String FILE_SCHEME = "file:///";
Additionally, could we add a test to ensure this change is verified and behaves as expected?
There was a problem hiding this comment.
+1, it should definitely be file:/// not file://.
However the :/// is not part of the scheme so that specific constant might be confusing.
There was a problem hiding this comment.
Make sense. We could give a different constant name, like FILE_SCHEMA_PATH_PRFIX, or LOCAL_PATH_PREFIX.
There was a problem hiding this comment.
Thanks for the comments updated for prefix file:/// and testing
|
Side note: Should we standardize the location string when creating a catalog? For instance, Polaris converts user input like |
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java
Show resolved
Hide resolved
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java
Outdated
Show resolved
Hide resolved
Ensure matching file prefixes when using polaris instance with FILE based storage.
Description
We encountered a bug when attempting to register tables to a file-based instance of Polaris for testing. Through logging, we discovered an inconsistent file naming convention when comparing file:/// to /. This inconsistency caused the system to fail to recognize paths that were actually children as child paths, resulting in the isChildOf method failing every time.
This change ensures that when comparing file paths in the isChildOf method, we use a consistent prefix. Additionally, this change enhances the robustness of the method by ensuring that comparisons are made between paths with similar prefixes, whether they start with / or file:///.
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Please delete options that are not relevant.