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

feature(query): fix read quoted string #5870

Merged
merged 8 commits into from
Jun 9, 2022

Conversation

sundy-li
Copy link
Member

@sundy-li sundy-li commented Jun 9, 2022

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

Summary

Summary about this PR

Changelog

  • Bug Fix

Related Issues

Fixes #issue

@sundy-li sundy-li requested a review from b41sh June 9, 2022 08:31
@vercel
Copy link

vercel bot commented Jun 9, 2022

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

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Jun 9, 2022 at 3:01PM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Jun 9, 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 9, 2022
@b41sh
Copy link
Member

b41sh commented Jun 9, 2022

we should also add some unit tests for test_parse_value_source

@sundy-li
Copy link
Member Author

sundy-li commented Jun 9, 2022

@ZeaLoVe Seems the back quoted logic is incorrect in logic test.

AssertionError: Expected:
1 1 1
2 2 \"2\"-\"2\"
 Actual:
        1        1        1
        2        2  "2"-"2"
 Statement:
Parsed Statement
    at_line: 15,
    s_type: Statement: query, type: IIT, query_type: IIT, label: ['mysql', 'http'], retry: False,
    suite_name: gen/09_fuse_engine/09_0001_remote_insert,
    text:
        SELECT * FROM t1;
    results: [(<re.Match object; span=(0, 10), match='---- mysql'>, 21, '1 1 1\n2 2 "2"-"2"'), (<re.Match object; span=(0, 9), match='---- http'>, 25, '1 1 1\n2 2 \\"2\\"-\\"2\\"')],

@wubx
Copy link
Member

wubx commented Jun 9, 2022

/LGTM

self.keep_read(buf, |b| b != quota)?;
self.must_ignore_byte(quota)

loop {
Copy link
Member

@zhang2014 zhang2014 Jun 9, 2022

Choose a reason for hiding this comment

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

CSV format also need this. We can implement it in other pr

@BohuTANG
Copy link
Member

BohuTANG commented Jun 9, 2022

@ZeaLoVe test_sqllogic_standalone_linux failure, please take a look, thanks.

@ZeaLoVe
Copy link
Contributor

ZeaLoVe commented Jun 9, 2022

cc @youngsofun , seams http handler return 1 when expect bool

@ZeaLoVe
Copy link
Contributor

ZeaLoVe commented Jun 9, 2022

https://github.com/datafuselabs/databend/blob/main/tests/logictest/suites/gen/09_fuse_engine/09_0001_remote_insert#L25 this case need change into:

statement query IIT
SELECT * FROM t1;

---- 
1 1 1
2 2 "2"-"2"

@youngsofun
Copy link
Member

log

handler return 1 when expect bool

return "true" on main on my mac

and logic test can pass

(venv) ➜  logictest git:(fix) curl --request POST '127.0.0.1:8001/v1/query/' --header 'Content-Type: application/json' --data-raw '{"sql": "select true", "string_fields":true}' -u root:

{"id":"d601cd71-7a8e-4e31-862b-283ca2b422d9","session_id":"ea35c032-3b36-4cc4-a4b8-30445b9417d3","schema":{"fields":[{"name":"true","default_expr":null,"data_type":{"type":"Boolean"}}],"metadata":{}},"data":[["true"]],"state":"Succeeded","error":null,"stats":{"scan_progress":{"rows":1,"bytes":1},"running_time_ms":7.6482909999999995},"stats_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9","final_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9/final","next_uri":null,"kill_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9/kill"}% 

try rebase?

@youngsofun
Copy link
Member

https://github.com/sundy-li/databend/commits/fix-quoted

@sundy-li the commit not include my pr

@BohuTANG
Copy link
Member

BohuTANG commented Jun 9, 2022

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Jun 9, 2022

update

✅ Branch has been successfully updated

@BohuTANG
Copy link
Member

BohuTANG commented Jun 9, 2022

log

handler return 1 when expect bool

return "true" on main on my mac

and logic test can pass

(venv) ➜  logictest git:(fix) curl --request POST '127.0.0.1:8001/v1/query/' --header 'Content-Type: application/json' --data-raw '{"sql": "select true", "string_fields":true}' -u root:

{"id":"d601cd71-7a8e-4e31-862b-283ca2b422d9","session_id":"ea35c032-3b36-4cc4-a4b8-30445b9417d3","schema":{"fields":[{"name":"true","default_expr":null,"data_type":{"type":"Boolean"}}],"metadata":{}},"data":[["true"]],"state":"Succeeded","error":null,"stats":{"scan_progress":{"rows":1,"bytes":1},"running_time_ms":7.6482909999999995},"stats_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9","final_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9/final","next_uri":null,"kill_uri":"/v1/query/d601cd71-7a8e-4e31-862b-283ca2b422d9/kill"}% 

try rebase?

Comments @mergify update will do the rebase to main.

@youngsofun
Copy link
Member

youngsofun commented Jun 9, 2022

after rebase sundy`s branch on my mac

the 1/true does not faile again

but the \"2\"-\"2\" need to change to 2 2 "2"-"2" as @ZeaLoVe said, seems to be fixed by this pr (no sure why)

@sundy-li

@wubx
Copy link
Member

wubx commented Jun 9, 2022

nice

@youngsofun
Copy link
Member

@sundy-li logic test passed
stateless test fail:

03_0016_insert_into_values:                                             [ FAIL ] - result differs with:
--- /workspace/tests/suites/0_stateless/03_dml/03_0016_insert_into_values.result	2022-06-09 14:47:07.250882209 +0000
+++ /workspace/tests/suites/0_stateless/03_dml/03_0016_insert_into_values.stdout	2022-06-09 14:51:12.930006462 +0000
@@ -1,5 +1,5 @@
 -1	33	2021-08-15 10:00:00.000000	string1234
 101	67	2021-11-15 10:00:00.000000	string5678
 100	100
-a"b"c'd
 a"b"c\\'d
+a"b"c'd

@mergify mergify bot merged commit f1935c4 into databendlabs:main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-bugfix this PR patches a bug in codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants