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

compatibility mysql insert and select #4883

Merged

Conversation

TCeason
Copy link
Collaborator

@TCeason TCeason commented Apr 16, 2022

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

Summary

compatibility double quoted for MySQL:

insert into t values("xxx");
show tables where table_name like "";
show databases where name like "";
select "";
select * from t where name = "";
select * from t2 where id in ("1");
select * from t2 where c between "" and "";
select * from t2 where id like "";

Changelog

  • Improvement
  • Build/Testing/CI

Related Issues

Fixes #4861 #1551

@vercel
Copy link

vercel bot commented Apr 16, 2022

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

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Apr 17, 2022 at 1:51AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 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 pr-improvement pr-build this PR changes build/testing/ci steps labels Apr 16, 2022
@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from 499ea9a to baea4ec Compare April 16, 2022 07:45
@BohuTANG BohuTANG requested a review from sundy-li April 16, 2022 08:34
@BohuTANG
Copy link
Member

BohuTANG commented Apr 16, 2022

The error is:

Traceback (most recent call last):
  File "/workspace/tests/suites/0_stateless/10_drivers/10_0000_python_clickhouse_driver.py", line 6, in <module>
    from clickhouse_driver import connect
ModuleNotFoundError: No module named 'clickhouse_driver'

For new setup tools, we should open a PR for clickhouse_driver first, then merge to this PR after main branch docker build finished, there is a demo(install sqlalchemy module a few days ago) : #4775

TCeason added a commit to TCeason/datafuse that referenced this pull request Apr 16, 2022
@BohuTANG
Copy link
Member

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2022

update

✅ Branch has been successfully updated

use databend_query::sql::DfParser;
use honggfuzz::fuzz;

fn main() {
loop {
fuzz!(|data: String| {
let _ = DfParser::parse_sql(&data);
let _ = DfParser::parse_sql(&data, SessionType::Fuzz);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, how to run fuzz?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I also have some issues with this file. I'm not sure why we need this file. And can not find where it is used.

To build success, I add a new SessionType Fuzz.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @PsiACE Are we using this for testing now? And how can I use it for databend query?

Copy link
Member

Choose a reason for hiding this comment

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

Fuzz type is not necessary, I think it makes more sense to use it for fuzz testing of existing session types. As for how to run the fuzz test, you can see https://github.com/datafuselabs/databend/blob/main/tools/fuzz/README.md

It is not integrated in CI for now, and I am not sure if there are plans to use it to improve our parser. We have introduced the fuzz test in #700. It has helped us in the past to make some small improvements to the parser or the upstream sqlparser.

@BohuTANG BohuTANG requested a review from b41sh April 16, 2022 10:04
Ok(result)
pub fn parse_sql(
sql: &'a str,
typ: SessionType,
Copy link
Member

Choose a reason for hiding this comment

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

If databend default sql dialect is MySqlDialect, the problems is?

Copy link
Member

@BohuTANG BohuTANG Apr 16, 2022

Choose a reason for hiding this comment

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

Then we no need a SessionType there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If use ClickHouse Client connect Databend server or ClickHouse server SQL like

select “number” will return col value

But use MySQL client it will return a string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests/suites/0_stateless/00_dummy/00_0000_dummy_select_1.sql

like this test

Copy link
Member

Choose a reason for hiding this comment

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

Double quota is not sql standard(not allowed by Snowflake, Pg, ClickHouse), it's just for MySQL...

So we need this special SessionType

query/src/sql/sql_parser.rs Show resolved Hide resolved
query/src/sql/statements/value_source.rs Show resolved Hide resolved
@@ -37,7 +37,7 @@ select * from t3;
select arr[0] from t3;
select arr[5] from t3;
select arr[3][0] from t3;
select arr[4]["k"] from t3;
Copy link
Member

Choose a reason for hiding this comment

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

Do double-quoted strings cause errors?

@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from 391e891 to 11c1772 Compare April 16, 2022 10:34
@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from 11c1772 to ea57906 Compare April 16, 2022 10:36
@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from ea57906 to 87870ed Compare April 16, 2022 11:23
@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from 87870ed to d8a7802 Compare April 16, 2022 11:25
compatibility double quoted for MySQL:

```
insert into t values("xxx");
select * from t where col="xxx";
```
@TCeason TCeason force-pushed the ISSUE-4861/compatibility_mysql_select branch from d8a7802 to 2f95750 Compare April 17, 2022 01:36
@BohuTANG
Copy link
Member

BohuTANG commented Apr 17, 2022

Tips:
When the PR review starts, please don't use force-pushed, because it loses a lot of information and the reviewer doesn't know what was changed from the last change.

Copy link
Member

@BohuTANG BohuTANG left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-build this PR changes build/testing/ci steps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility MySQL insert
6 participants