grafana / clickhouse-datasource

Grafana Plugin for ClickHouse
Apache License 2.0
136 stars 64 forks source link

SQL parsing into an AST fails on INTERVAL and SAMPLE statements #958

Open eKrajnak opened 2 months ago

eKrajnak commented 2 months ago

What happened: Clickhouse query which contains:

What you expected to happen: Apply adhoc filter and perform query

How to reproduce it (as minimally and precisely as possible):

Apply any adhoc filter and run the query:

SELECT *
FROM data
SAMPLE 1/10
WHERE time > NOW() - INTERVAL 1 MINUTE

and watch javascript console log:

Unexpected int token: "1". Instead, I was expecting to see one of the following:

    - A "lparen" token
    - A "kw_cross" token
    - A "kw_left" token
    - A "kw_right" token
    - A "kw_full" token
    - A "comma" token
    - A "kw_inner" token
    - A "kw_where" token
    - A "kw_join" token
    - A "kw_group" token
    - A "kw_order" token
    - A "kw_offset" token
    - A "kw_for" token
    - A "kw_limit" token
    - A "kw_fetch" token
    - A "kw_union" token
    - A "semicolon" token

Environment:

SpencerTorres commented 2 months ago

The AST is notorious for having SQL syntax problems. I'll try to reproduce this locally and see if it can be patched.

@bossinc We might need to start exploring other options for the AST. I believe one of its main purposes is for extracting the table name (at least for adhoc filters). Maybe we can add a field to the SQL UI where people can manually write the table name. Or we can find a good Go module on the backend to handle it and use that to get an AST via HTTP request to the plugin.

eKrajnak commented 2 months ago

Thank you. For INTERVAL 1 MINUTE I can use workaround "toIntervalMinute(1)" already now. But for SAMPLE I have no solution. I took a fast look on AST. If I'm correct it's AST plugin for Postgresql and it knows "TABLESAMPLE" with the very similar meaning. Maybe it could be just renamed.

SpencerTorres commented 2 months ago

The AST plugin is pretty flexible actually, maybe we can just add the keyword somewhere. Thanks for mentioning that

eKrajnak commented 1 month ago

Any update about the issue?

SpencerTorres commented 1 month ago

No update yet, although I've been looking into alternatives for the AST parser since this is just one of many issues related to the current implementation