-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[HUDI-7367] Add makeQualified APIs #10607
[HUDI-7367] Add makeQualified APIs #10607
Conversation
@linliu-code to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments. Willing to concede for now, given its matching the hadoop APIs closely
if (!isAbsolute()) { | ||
throw new IllegalStateException("Only an absolute path can be made qualified"); | ||
} | ||
HoodieLocation location = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found this a bit weird. the assignment from this
. is that needed?
* a new path that includes a path and authority and is fully qualified. | ||
*/ | ||
@PublicAPIMethod(maturity = ApiMaturityLevel.EVOLVING) | ||
public HoodieLocation makeQualified(URI defaultUri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ought to be a static method? I think we should instead make this a new constructor in HoodieLocation
?
Change Logs
This PR makes the following changes to properly support qualified location:
getUri
toHoodieStorage
.makeQualified
toHoodieLocation
.makeQualified
toFSUtils
.Impact
Properly support qualified location with
HoodieStorage
andHoodieLocation
. This fixes the original test failures in #10591.Risk level
none
Documentation Update
N/A
Contributor's checklist