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.

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

feat: Introduce FileIO #53

Merged
merged 7 commits into from
Sep 20, 2023
Merged

feat: Introduce FileIO #53

merged 7 commits into from
Sep 20, 2023

Conversation

liurenjie1024
Copy link
Contributor

Close #49

In this pr we implement FileIO on top of opendal, which provided great abstraction over all kinds of storage. There are still some missing check, but I want to discuss implementation first.

@liurenjie1024
Copy link
Contributor Author

cc @ZENOTME @Fokko @JanKaul @Xuanwo PTAL

crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/error.rs Outdated Show resolved Hide resolved
@liurenjie1024
Copy link
Contributor Author

I've completed the implementation. PTAL cc @Xuanwo @JanKaul @ZENOTME @Fokko

crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
@liurenjie1024
Copy link
Contributor Author

Any other comments? cc @Fokko @Xuanwo @JanKaul @ZENOTME

crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
crates/iceberg/src/io.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

LGTM, let's go!

Copy link
Collaborator

@JanKaul JanKaul left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

Copy link
Contributor

@ZENOTME ZENOTME left a comment

Choose a reason for hiding this comment

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

Thanks! Nice work!

@liurenjie1024
Copy link
Contributor Author

cc @Fokko I think it's ok to merge?

@Fokko
Copy link
Contributor

Fokko commented Sep 20, 2023

@liurenjie1024 I believe so, thanks for the review, everyone! 🙌🏻

@Fokko Fokko merged commit 8c55de4 into apache:main Sep 20, 2023
6 checks passed
@liurenjie1024 liurenjie1024 deleted the renjie/issue-49 branch September 20, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement FileIO interface.
6 participants