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

[Enhancement] in SQL query, treat -- inside quotes not as comment #374

Closed
delphid opened this issue Oct 14, 2021 · 5 comments · Fixed by #387
Closed

[Enhancement] in SQL query, treat -- inside quotes not as comment #374

delphid opened this issue Oct 14, 2021 · 5 comments · Fixed by #387
Milestone

Comments

@delphid
Copy link
Contributor

delphid commented Oct 14, 2021

behavior now

--within quotes is also treated as comment. This brings unexpected truncation of query.

  • This happens when store Pytest case results in Clickhouse and then query. In Pytest, when parametrize cases with sets of values [('name1', 'id1', 'score1'), ('name2', 'id2', 'score2'), ('name3', '', 'score3')], case name will become like name1-id1-score1. Say if id is omitted for the 3rd case, case name then becomes name3--score3, then in query:
    • when query: SELECT * FROM my_table WHERE case='name3--score3'
    • then generated SQL: SELECT * FROM my_db.my_table WHERE case='name3

desired behavior

  • when query: SELECT * FROM my_table WHERE case='name3--score3'-- some comment
  • then generated SQL: SELECT * FROM my_db.my_table WHERE case='name3--score3'

proposed change

https://github.com/delphid/clickhouse-grafana/blob/commentRe/src/scanner.ts#L304
image

demo

const comment_reg = '--(([^\"\']*[\"\']){2})*[^\"\']*[$\n]';
const query = 'query content"--actually not comment" \'--also not comment\'--some comment"comment in quotes" \'also comment\'\na new line of query';


console.log(
    query.replace(new RegExp(comment_reg, 'g'), '')
);


// result: query content"--actually not comment" '--also not comment'a new line of query

If that seems alright I'd like to submit a PR

@Slach
Copy link
Collaborator

Slach commented Oct 14, 2021

thanks a lot for reporting
could you make PR and add new test case into spec/scanner_specs.jest.ts
use docker-compose run --rm frontend_builder
to run tests

@Slach
Copy link
Collaborator

Slach commented Oct 14, 2021

Also, could you provide, SQL query example when you need -- as non comment?

@delphid
Copy link
Contributor Author

delphid commented Oct 14, 2021

Simplified use case: see the behavior now part in Pytest related contents.

My real use case looks like this:

select timestamp, suite, case, duration, result, log, failure from my_db.my_table
where
  timestamp >= $from and timestamp <= $to
  and business in ($business)
  and system in ($system)
  and suite in ($suite)
  and case in ($case)
  and result in ($result)
  and env='$env'
order by timestamp desc
limit $limit

It happens in the and case in ($case) part. When ($case) is something like ('aaa-5-mtdnn_xxx', 'bbb-10-bert_xxx', 'ccc--bert_yyy')

It's a Pytest feature to create name for parametrized case by connecting parameter values with -. When one parameter is omitted, will have '-' + '' + '-' -> '--'

cases are like:

tokens branch_id model_tag
aaa 5 mtdnn_xxx
bbb 10 bert_xxx
ccc bert_yyy

Thanks. I'll fill some test cases and run

@delphid
Copy link
Contributor Author

delphid commented Oct 14, 2021

Just added test case and used docker-compose run --rm frontend_builder to run tests to pass. PR is as follows:
https://github.com/Vertamedia/clickhouse-grafana/pull/375/files

@jonny-gg
Copy link

What can be published for this special character? It seems that the clickHouse plugin has not been published for a long time

@Slach Slach added this to the 2.4.0 milestone Nov 29, 2021
Slach added a commit to Altinity/grafana-plugin-repository that referenced this issue Nov 29, 2021
# 2.4.0 (2021-11-29)

## Enhancement:
* Add support for Grafana 8.x unified alerts, fix Altinity/clickhouse-grafana#380
* Add TLS support for backend alerts part of plugin Altinity/clickhouse-grafana#356 (comment)
* Add $naturalTimeSeries macro, look details in https://github.com/Vertamedia/clickhouse-grafana/pull/89/files#diff-cd9133eda7b58ef9c9264190db4534a1be53216edbda9ac57256fbd800368c03R383-R412
* Update golang-plugin-sdk-go to latest version
* Properly format Value in Table format, look details Altinity/clickhouse-grafana#379
* Remove toDateTime64 casting for column when time column is already DateTime64 to improve performance. Change test to ensure the casting is removed from the query, fix Altinity/clickhouse-grafana#360
* implements `$timeFilter64ByColumn(column_name)` macro, fix Altinity/clickhouse-grafana#343

## Fixes:

* implements properly GET and POST support for alert queries, fix Altinity/clickhouse-grafana#353
* SQL syntax highlight now works always, fix Altinity/clickhouse-grafana#174, fix Altinity/clickhouse-grafana#381
* fix Altinity/clickhouse-grafana#376,
* fix negative behavior for $perSecondColumns Altinity/clickhouse-grafana#337
* fix Altinity/clickhouse-grafana#374, ignore `--` inside quotas as comment
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