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

fix(query): Fix compressed buf not consumed correctly #5727

Merged
merged 13 commits into from
Jun 4, 2022
Merged

fix(query): Fix compressed buf not consumed correctly #5727

merged 13 commits into from
Jun 4, 2022

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jun 1, 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

fix(query): Fix compressed buf not consumed correctly

Changelog

  • Bug Fix

@Xuanwo
fix(query): Fix compressed buf not consumed correctly
8b64fac 
Signed-off-by: Xuanwo <github@xuanwo.io>
@vercel
Copy link

vercel bot commented Jun 1, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Jun 4, 2022 at 3:30AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2022

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.

@mergify mergify bot added the pr-bugfix this PR patches a bug in codebase label Jun 1, 2022
@BohuTANG BohuTANG requested a review from sundy-li June 1, 2022 11:35
@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 1, 2022

Maybe need to wait for @zhang2014 's PR.

So that he doesn't need to resolve conflicts again.

@Xuanwo
Merge remote-tracking branch 'origin/main' into fix-compress
84da42a 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo requested a review from zhang2014 June 2, 2022 08:24
@Xuanwo
Performance improve
627f0c2 
Signed-off-by: Xuanwo <github@xuanwo.io>
@BohuTANG
Copy link
Member

BohuTANG commented Jun 2, 2022 

<Backtrace disabled by default. Please use RUST_BACKTRACE=1 to enable> 
[ FAIL ] - result differs with:
--- /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0001_upload_to_stage.result	2022-06-02 09:22:34.945052455 +0000
+++ /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0001_upload_to_stage.stdout	2022-06-02 09:25:55.253876497 +0000
@@ -1 +1,2 @@
-00_0001_upload_to_stage.sh
+ERROR 1105 (HY000) at line 1: Code: 1068, displayText = Cannot join handle from context's runtime, cause: task 3180 panicked.
+ERROR 1105 (HY000) at line 1: Code: 1068, displayText = Cannot join handle from context's runtime, cause: task 3205 panicked.

@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 2, 2022 

<Backtrace disabled by default. Please use RUST_BACKTRACE=1 to enable> 
[ FAIL ] - result differs with:
--- /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0001_upload_to_stage.result	2022-06-02 09:22:34.945052455 +0000
+++ /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0001_upload_to_stage.stdout	2022-06-02 09:25:55.253876497 +0000
@@ -1 +1,2 @@
-00_0001_upload_to_stage.sh
+ERROR 1105 (HY000) at line 1: Code: 1068, displayText = Cannot join handle from context's runtime, cause: task 3180 panicked.
+ERROR 1105 (HY000) at line 1: Code: 1068, displayText = Cannot join handle from context's runtime, cause: task 3205 panicked.

This bug related to #4491, I will fix them at the same time

@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 2, 2022

apache/opendal#327 will fix this issue.

@Xuanwo
Fix list empty dir
0d96c46 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Make udeps happy
c0fe268 
Signed-off-by: Xuanwo <github@xuanwo.io>
@wubx
Copy link
Member

wubx commented Jun 3, 2022

Should we merge it?

@BohuTANG
Copy link
Member

BohuTANG commented Jun 3, 2022

test_stateful_standalone

It's not ready yet, there are some stateful-test failed and related to this PR:

The failure tests:
    /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0000_copy_from_s3_location.sh
    /home/runner/work/databend/databend/tests/suites/1_stateful/00_copy/00_0000_copy_from_stage.sh

@Xuanwo
debug
39dcd12 
Signed-off-by: Xuanwo <github@xuanwo.io>
BohuTANG
BohuTANG reviewed Jun 3, 2022
View reviewed changes
query/src/servers/http/v1/stage.rs
@@ -49,6 +48,8 @@ pub async fn upload_to_stage(

let user_mgr = context.get_user_manager();

// TODO(xuanwo): logic here seems buggy, we need to fix it.
// It's incorrect to get operator from context if we are uploading an external stage.
Copy link
Member

@BohuTANG BohuTANG Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Add new function as get_external_operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

These logics are not covered by stateful tests. So I think they are wrong for a long time.

I will fix them in another along with stateful tests.

@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 3, 2022 •
edited
Loading

I will refactor path-related logic after this bug fix.

  • Abstract more robust utils functions
  • Adding unit tests cases for them
  • Setup a clear rule and docs for path handling

@Xuanwo
Fix list file
31c12a3 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix stage parse
92b658e 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix path not handled correctly
46dccc3 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Merge remote-tracking branch 'origin/main' into fix-compress
32f4bd9
@Xuanwo
Add comment for location_to_stage_path
2cba88e 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Make clippy happy
6e13176 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Fix unit test
fe5cc30 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Member Author

Xuanwo commented Jun 4, 2022

@BohuTANG This PR is ready to go

@BohuTANG BohuTANG merged commit 87ad8f1 into databendlabs:main Jun 4, 2022
@Xuanwo Xuanwo deleted the fix-compress branch June 4, 2022 08:10
@wubx
Copy link
Member

wubx commented Jun 5, 2022

Awesome.

ls ./dataset/*.csv.gz|xargs -P 8 -I{} curl -H "insert_sql:insert into ontime format CSV" -H "skip_header:1"   -H "compression:gzip" -F "upload=@{}" -XPUT http://root:@localhost:8000/v1/streaming_load

can work too.

@Xuanwo Xuanwo mentioned this pull request Jun 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sundy-li sundy-li sundy-li left review comments

@BohuTANG BohuTANG BohuTANG left review comments

@zhang2014 zhang2014 zhang2014 approved these changes

Assignees
No one assigned
Labels
need-review pr-bugfix this PR patches a bug in codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Xuanwo @BohuTANG @wubx @sundy-li @zhang2014 @databend-bot