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(core)!: make list return path itself #4959

Merged
merged 21 commits into from
Aug 26, 2024
Merged

Conversation

meteorgan
Copy link
Contributor

Which issue does this PR close?

Closes #4877

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@meteorgan
Copy link
Contributor Author

hi, @Xuanwo I have finished the work for FS and S3, but I’m not sure if the implementation is elegant, especially for S3. Do you have any advice? If you think it’s okay, I’ll continue to complete the others.

@@ -146,11 +146,9 @@ impl typed_kv::Adapter for Adapter {
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this right_range isn't precise, {path}{char::MAX} should include. maybe we should set right bound unbound, and filter by prefix ?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to solve it in the next PR. just don't want to add too much work in this one.
ps: this PR is almost done

Copy link
Member

Choose a reason for hiding this comment

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

Thank you very much for your effort. This issue is truly complex and time-consuming.

@Xuanwo Xuanwo changed the title feat(core): make list return path itself feat(core)!: make list return path itself Aug 21, 2024
@meteorgan
Copy link
Contributor Author

The HDFS tests seem flaky. In the latest commit, they failed, but the changes aren’t related to them.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 22, 2024

The HDFS tests seem flaky. In the latest commit, they failed, but the changes aren’t related to them.

Yes. The ASF download CDN returns 502. Please ignore it. Maybe we can try cache them if you are interested.

@meteorgan meteorgan marked this pull request as ready for review August 22, 2024 15:43
@meteorgan
Copy link
Contributor Author

The HDFS tests seem flaky. In the latest commit, they failed, but the changes aren’t related to them.

Yes. The ASF download CDN returns 502. Please ignore it. Maybe we can try cache them if you are interested.

not this one. https://github.com/apache/opendal/actions/runs/10486150908/job/29043772467, here only test_list_empty_dir failed

@Xuanwo
Copy link
Member

Xuanwo commented Aug 22, 2024

not this one. apache/opendal/actions/runs/10486150908/job/29043772467, here only test_list_empty_dir failed

No idea so far. The test and the code seems correct.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 22, 2024

Thank you very much for this PR. I will take a review tomorrow.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 23, 2024

This PR looks good to me overall. We will merge it after the release of version 0.49.2.

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.

Thanks!

@Xuanwo Xuanwo merged commit 74a3e29 into apache:main Aug 26, 2024
227 checks passed
@meteorgan meteorgan deleted the list_prefix branch September 20, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idea: Make List return the path itself
2 participants