Skip to content

Query Builder: Column names containing parentheses not handled correctly #1217

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

Open
mbUSC opened this issue Apr 9, 2025 · 3 comments
Open
Labels
datasource/ClickHouse type/bug Something isn't working

Comments

@mbUSC
Copy link

mbUSC commented Apr 9, 2025

(cross-posting from 103501 in the Grafana repo)

What happened?

If a column name contains parentheses, the query builder fails to generate the correct SQL statements.

For example, let's say we have a Clickhouse data source that has a table with the following columns:

  • Track ID
  • Length (Kilometers)

When selecting those two columns, the query builder generates this SQL:

SELECT "Track ID", Length (Kilometers) FROM "mydatabase" . "TracksTable" LIMIT 1000

What did you expect to happen?

The correct SQL should be generated (i.e. Length (Kilometers) should be enclosed in double quotes):

SELECT "Track ID", "Length (Kilometers)" FROM "mydatabase" . "TracksTable" LIMIT 1000

Did this work before?

Not sure

How do we reproduce it?

See example provided above

Is the bug inside a dashboard panel?

No response

Environment (with versions)?

Grafana: 11.6.0
OS: MacOS
Browser: Safari

Grafana platform?

Kubernetes

Datasource(s)?

Clickhouse Datasource plugin

@mbUSC
Copy link
Author

mbUSC commented Apr 11, 2025

@SpencerTorres

Looks like this is the offending code:
https://github.com/grafana/clickhouse-datasource/blob/00710672ed9e97eb59070b7e4131fa26ea1bea31/src/data/sqlGenerator.ts#L539C1-L555C1

const getColumnIdentifier = (col: SelectedColumn): string => {
  let colName = col.name;


  // allow for functions like count()
  if (colName.includes('(') || colName.includes(')') || colName.includes('"') || colName.includes('"') || colName.includes(' as ')) {
    colName = col.name
  } else if (colName.includes(' ')) {
    colName = escapeIdentifier(col.name);
  }


  if (col.alias && (col.alias !== col.name && escapeIdentifier(col.alias) !== colName)) {
    return `${colName} as "${col.alias}"`
  }


  return colName;
}

We should be able to logically separate out the function from the column name, since they are two different things

@SpencerTorres
Copy link
Collaborator

Hello! Yep that's the exact code that came to mind when I read the title.

We should be able to logically separate out the function from the column name, since they are two different things

There's many edge cases to consider. I think in this case the best workaround is to manually add the quotes when selecting the column. There's too many cases where users are trying to write functions and they get escaped with quotes. You should be able to manually type in the column name with quotes. I understand this is inconvenient. If you have any suggestions on how we can best accommodate both, I'm open to ideas and PRs.

Thanks!

@mbUSC
Copy link
Author

mbUSC commented Apr 12, 2025

Are there any places where the user can actually type the function names, other than the SQL editor itself?

All I see in the Query Builder is drop-down menus:

Image

I am trying to think what feature would fail if all column names were escaped

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datasource/ClickHouse type/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants