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

Query wrongly gets appended "FORMAT TabSeparatedWithNamesAndTypes" #168

Closed
ClementPinard opened this issue Feb 23, 2022 · 3 comments · Fixed by #180
Closed

Query wrongly gets appended "FORMAT TabSeparatedWithNamesAndTypes" #168

ClementPinard opened this issue Feb 23, 2022 · 3 comments · Fixed by #180

Comments

@ClementPinard
Copy link

ClementPinard commented Feb 23, 2022

Describe the bug
I have been using clickhouse-sqlalchemy for some time without any problem. Not sure exactly why, but I have always used my query with a FORMAT TabSeparatedWithNamesAndTypes in the end, even though it was probably not necessary.

I recently updated my package from 0.1.4 to 0.2.0 and now i get the following error:

DatabaseException: Orig exception: Code: 62, e.displayText() = DB::Exception: Syntax error: failed at position 887 ('FORMAT') (line 19, col 38): FORMAT TabSeparatedWithNamesAndTypes. Expected one of: end of query, SETTINGS (version 21.3.13.9 (official build))

After some investigation, it seems that the problem is that the query gets appended FORMAT TabSeparatedWithNamesAndTypeseven though it's already there, and it's gets the driver unhappy to see this line twice. Some debugging made me believe the format string gets appended here : https://github.com/xzkostyan/clickhouse-sqlalchemy/blob/master/clickhouse_sqlalchemy/drivers/http/base.py#L18

Is there a reason why we don't check if FORMAT is in the statement string here contrary to here https://github.com/xzkostyan/clickhouse-sqlalchemy/blob/master/clickhouse_sqlalchemy/drivers/http/connector.py#L112 ?

I know that just removing this line from my queries makes the problem away so there's no urgency, but I thought you should know. No sure why it didn't trigger any error before also, maybe it happened and the parser did not raise Syntaxe error befor, I have not investigated with the former package versions

To Reproduce
Any query with FORMAT TabSeparatedWithNamesAndTypes should raise the error I believe.

Expected behavior
Format suffix should not be appended here since it's already in the query.

Versions

  • package version 0.2.0
  • python version 3.9
@xzkostyan
Copy link
Owner

Well, we check for FORMAT is in the statement: 'FORMAT' not in raw_sql_big.

Why do we need add FORMAT clause to the query explicitly outside the library? It can lead to errors during response processing. FORMAT CSV response for example cannot be parsed.

@ClementPinard
Copy link
Author

Well, we check for FORMAT is in the statement: 'FORMAT' not in raw_sql_big.

You only check it here https://github.com/xzkostyan/clickhouse-sqlalchemy/blob/master/clickhouse_sqlalchemy/drivers/http/connector.py#L112

but you don't check it here : https://github.com/xzkostyan/clickhouse-sqlalchemy/blob/master/clickhouse_sqlalchemy/drivers/http/base.py#L18 and I am pretty that for my sql code, this is where it wrongly gets appended.

Also, kinda agree with your second statement, could this be clearly stated somewhere ? Before 0.1.6 I believe that you did need to add the FORMAT TabSeparatedWithNamesAndTypes for it to work properly, and now it breaks with a hard to read error message.

Seems to me that the best would be to be robust to an already existing FORMAT for backward compatibilities, and raise some explicit error when the FORMAT is incompatible, like FORMAT CSV.

@alexeiser
Copy link

We have also hit this issue when trying to perform a create user sql call - which fails because it also has FORMAT TabSeparatedWithNamesAndTypes appended which is not allowed in the clickhouse SQL syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants