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

API: Deprecate ContentFile#path API and add location API which returns String #11092

Merged
merged 2 commits into from
Sep 23, 2024
Merged

API: Deprecate ContentFile#path API and add location API which returns String #11092

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80fec6e
API: Deprecate ContentFile#path API and add location API which return…
amogh-jahagirdar Sep 6, 2024
ea7d2ec
Update api/src/main/java/org/apache/iceberg/ContentFile.java
amogh-jahagirdar Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion api/src/main/java/org/apache/iceberg/ContentFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,19 @@ public interface ContentFile<F> {
*/
FileContent content();

/** Returns fully qualified path to the file, suitable for constructing a Hadoop Path. */
/**
* Returns fully qualified path to the file, suitable for constructing a Hadoop Path.
*
* @deprecated since 1.7.0, will be removed in 2.0.0 Use {@link #location()} instead.
amogh-jahagirdar marked this conversation as resolved.
Show resolved Hide resolved
*/
@Deprecated
CharSequence path();

/** Return the fully qualified path to the file, suitable for constructing a Hadoop Path */
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we still want to keep the Hadoop Path reference here as it quite evolved over the years and we do not necessarily depend on Hadoop anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I was largely concerned if we didn't preserve this property of being suitable for a hadoop path at the API level, once path is deprecated, it may break the users that do rely on having this. But if we're saying that we don't really need to concern ourselves on maintaining this, we may not need to include it. cc @rdblue thoughts on this?

Copy link
Contributor Author

@amogh-jahagirdar amogh-jahagirdar Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Discussed with @rdblue @danielcweeks and here are some key points on why we shouldn't document anything related to hadoop:

  1. Hadoop pathing doesn't work with storage systems like GCS
  2. Locations are a super set of what can be represented in a hadoop path. Even today path API can return paths which simply cannot be represented in a hadoop Path.
  3. The hadoop path normalization logic simply conflicts with absolute paths

So updated to remove this part of the comment. I'll leave the existing path API comment as is since it's being deprecated anyways.

default String location() {
return path().toString();
}

/** Returns format of the file. */
FileFormat format();

Expand Down