Skip to content
This repository was archived by the owner on Nov 7, 2025. It is now read-only.

Conversation

@DongLiang-0
Copy link
Contributor

Previously, columns and tables in SQL used double quotes, but in doris syntax, columns and tables should use back quotes, otherwise they will be recognized as a string. This change is applicable to both clickhouse and doris

select "col1" from tab1 -> select `col1` from `tab1`

@DongLiang-0 DongLiang-0 requested a review from a team as a code owner July 22, 2025 12:47
@DongLiang-0 DongLiang-0 force-pushed the BackquoteIdentifier branch from 15ed231 to a920790 Compare July 22, 2025 12:47
Copy link
Member

@mieciu mieciu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution!

I think that instead of swapping " with `, this should be configurable, depending on the backend connector. While this syntax is fine with ClickHouse/Hydrolix, it's non supported in something like Postgres.

Therefore, there should be no need to change any of the existing test cases.

How I would see this change moving forward

We have this oversimplification of model.LiteralExpr being used for column/table names and aliases. This is the moment where we need to refactor that.

We need to create model.IdentifierExpr for our query AST which should replace model.LiteralExpr whenever referring to column/table name. Then, when rendering to SQL we should transform it accordingly to the database backend we're connected to. ClickHouse/Hydrolix -> let's stick to ". For Doris, we can use "`".

In that way, we won't have to change any of the existing tests, make adding different data sources easier and there's way less risk of introducing regression to the codebase.

@DongLiang-0
Copy link
Contributor Author

Thanks a lot for your contribution!

I think that instead of swapping " with `, this should be configurable, depending on the backend connector. While this syntax is fine with ClickHouse/Hydrolix, it's non supported in something like Postgres.

Therefore, there should be no need to change any of the existing test cases.

How I would see this change moving forward

We have this oversimplification of model.LiteralExpr being used for column/table names and aliases. This is the moment where we need to refactor that.

We need to create model.IdentifierExpr for our query AST which should replace model.LiteralExpr whenever referring to column/table name. Then, when rendering to SQL we should transform it accordingly to the database backend we're connected to. ClickHouse/Hydrolix -> let's stick to ". For Doris, we can use "`".

In that way, we won't have to change any of the existing tests, make adding different data sources easier and there's way less risk of introducing regression to the codebase.

very good plan

@DongLiang-0
Copy link
Contributor Author

I'll close this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants