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

dal2: Implement s3 basic operations #3821

Merged
merged 9 commits into from
Jan 11, 2022
Merged

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jan 10, 2022

Signed-off-by: Xuanwo github@xuanwo.io

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Part of #3774

Changelog

  • New Feature

Test Plan

Unit Tests

Stateless Tests

Signed-off-by: Xuanwo <github@xuanwo.io>
@databend-bot databend-bot added the pr-feature this PR introduces a new feature to the codebase label Jan 10, 2022
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@vercel
Copy link

vercel bot commented Jan 10, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/BEaiR5mdnPBjHuBi9FJfCt8yweeh
✅ Preview: https://databend-git-fork-xuanwo-dal-s3-basic-databend.vercel.app

@Xuanwo Xuanwo changed the title Implement s3 write dal2: Implement s3 write Jan 10, 2022
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as ready for review January 11, 2022 02:07
@Xuanwo Xuanwo requested a review from BohuTANG as a code owner January 11, 2022 02:07
@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 11, 2022

@dantengsky, thanks for help, now this PR is ready for review.

@Xuanwo Xuanwo changed the title dal2: Implement s3 write dal2: Implement s3 basic operations Jan 11, 2022
common/dal2/src/ops/io.rs Outdated Show resolved Hide resolved
common/dal2/src/ops/io.rs Outdated Show resolved Hide resolved
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #3821 (a1058d1) into main (d757471) will increase coverage by 0%.
The diff coverage is 83%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #3821    +/-   ##
======================================
  Coverage     60%     60%            
======================================
  Files        711     711            
  Lines      38335   38552   +217     
======================================
+ Hits       23008   23146   +138     
- Misses     15327   15406    +79     
Impacted Files Coverage Δ
common/dal2/src/services/s3.rs 68% <ø> (-6%) ⬇️
common/dal2/src/ops/io.rs 72% <83%> (+72%) ⬆️
query/src/common/mod.rs 70% <0%> (-14%) ⬇️
query/src/interpreters/interpreter_common.rs 40% <0%> (-7%) ⬇️
common/io/src/unmarshal.rs 95% <0%> (-5%) ⬇️
common/planners/src/plan_database_drop.rs 61% <0%> (-3%) ⬇️
common/meta/types/src/database.rs 93% <0%> (-3%) ⬇️
.../src/interpreters/interpreter_show_create_table.rs 95% <0%> (-3%) ⬇️
common/meta/types/src/table.rs 96% <0%> (-2%) ⬇️
query/src/storages/fuse/statistics/accumulator.rs 84% <0%> (-2%) ⬇️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d757471...a1058d1. Read the comment docs.

Copy link
Member

@dantengsky dantengsky left a comment

Choose a reason for hiding this comment

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

LGTM

@databend-bot
Copy link
Member

Wait for another reviewer approval

@Xuanwo Xuanwo requested review from Veeupup and PsiACE January 11, 2022 06:52
Copy link
Member

@PsiACE PsiACE left a comment

Choose a reason for hiding this comment

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

lgtm

@databend-bot
Copy link
Member

CI Passed
Reviewers Approved
Let's Merge
Thank you for the PR @Xuanwo

@mergify mergify bot merged commit b79fdfc into databendlabs:main Jan 11, 2022
@Xuanwo Xuanwo deleted the dal-s3-basic branch January 11, 2022 10:11
@Xuanwo Xuanwo mentioned this pull request Jan 11, 2022
4 tasks

use super::*;

#[tokio::test]
Copy link
Member

Choose a reason for hiding this comment

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

This test should move to test :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ReaderStream intends to be used internally in dal2 crate.

Do you think it's better to export it and then do integration testing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants