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

Jump to bottom

Core, Hive, Nessie: Use ResolvingFileIO as default instead of HadoopFileIO #8272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Core, Hive, Nessie: Use ResolvingFileIO as default instead of HadoopFileIO #8272

wants to merge 1 commit into from

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Aug 9, 2023

No description provided.

@nastra nastra force-pushed the resolving-file-io-as-default branch from 948a917 to fb23e4d Compare August 9, 2023 11:10
@github-actions github-actions bot added the flink label Aug 9, 2023
@nastra nastra force-pushed the resolving-file-io-as-default branch from fb23e4d to fcbc64e Compare August 9, 2023 12:57
bryanck
bryanck approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@bryanck bryanck left a comment

Choose a reason for hiding this comment

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

This looks good to me, with the assumption that #7976 will get merged.

@szehon-ho
Copy link
Collaborator

Also, can we make it support SupportsPrefixOperations. Thinking it a possible option for: #8194

@nastra
Copy link
Contributor Author

nastra commented Aug 10, 2023

Also, can we make it support SupportsPrefixOperations. Thinking it a possible option for: #8194

@szehon-ho did you mean to comment on #7976 w.r.t SupportsPrefixOperation rather than on this PR?

dramaticlly
dramaticlly approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

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

change LGTM, do we need to update docs anywhere to better inform user? Since this default value change and some might not be fully aware of

@nastra
Copy link
Contributor Author

nastra commented Aug 15, 2023

change LGTM, do we need to update docs anywhere to better inform user? Since this default value change and some might not be fully aware of

This change should be completely transparent for users

bryanck
bryanck reviewed Sep 13, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
@@ -67,6 +67,7 @@ public class JdbcCatalog extends BaseMetastoreCatalog
private static final String NAMESPACE_EXISTS_PROPERTY = "exists";
private static final Logger LOG = LoggerFactory.getLogger(JdbcCatalog.class);
private static final Joiner SLASH = Joiner.on("/");
private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO";
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the name from the class potentially, rather than a string literal, as ResolvingFileIO is in core.

@rdblue
Copy link
Contributor

rdblue commented Sep 13, 2023

I think we need to discuss this more before merging it. This will change the default FileIO implementation for a lot of people -- how are we confident that it won't cause problems? Also, what is the motivation for changing the default? Are we fixing something?

@danielcweeks danielcweeks added this to the Iceberg 2.0.0 milestone Sep 20, 2023
@bryanck bryanck mentioned this pull request Sep 25, 2023
Closed
@ajantha-bhat
Copy link
Member

I remember discussing this in the community sync. Why this didn't move forward?

@nastra
Copy link
Contributor Author

nastra commented Dec 12, 2023

@ajantha-bhat this will be done prior to releasing 2.0.0

@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 13, 2024
@nastra nastra removed the stale label Sep 13, 2024
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 15, 2024
@nastra nastra added not-stale and removed stale labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryanck bryanck bryanck approved these changes

@dramaticlly dramaticlly dramaticlly approved these changes

Assignees
No one assigned
Labels
core flink hive NESSIE not-stale
Projects
None yet
Milestone
Iceberg 2.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nastra @szehon-ho @rdblue @ajantha-bhat @bryanck @dramaticlly @danielcweeks