Skip to content

ClickHouse query runner: fixed error message#6764

Merged
justinclift merged 11 commits intogetredash:masterfrom
denisov-vlad:clickhouse-error-fix
Mar 17, 2024
Merged

ClickHouse query runner: fixed error message#6764
justinclift merged 11 commits intogetredash:masterfrom
denisov-vlad:clickhouse-error-fix

Conversation

@denisov-vlad
Copy link
Copy Markdown
Member

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

Since ~ 23.10 ClickHouse returns 200 status code for all types of requests, but adds exception in response. This PR adds conditions to check for this key.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

#6756

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 10, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 63.42%. Comparing base (7d0d242) to head (e47aa2d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6764      +/-   ##
==========================================
- Coverage   63.43%   63.42%   -0.01%     
==========================================
  Files         163      163              
  Lines       13200    13203       +3     
  Branches     1822     1823       +1     
==========================================
+ Hits         8373     8374       +1     
- Misses       4530     4531       +1     
- Partials      297      298       +1     
Files Coverage Δ
redash/query_runner/clickhouse.py 60.76% <40.00%> (-0.65%) ⬇️

@denisov-vlad
Copy link
Copy Markdown
Member Author

@getredash/maintainers could you merge it please? Now we don't see any messages on errors, it's difficult to debug errors in SQL.

@gaecoli gaecoli enabled auto-merge (squash) March 17, 2024 15:05
@gaecoli gaecoli disabled auto-merge March 17, 2024 15:06
@gaecoli
Copy link
Copy Markdown
Member

gaecoli commented Mar 17, 2024

Your branch has some conflict. I updated your branch, if CI have pass, i will merge it. Don't worry.

@justinclift
Copy link
Copy Markdown
Member

Looks like it's passing enough. Lets merge it. 😄

@justinclift justinclift merged commit 667a696 into getredash:master Mar 17, 2024
@denisov-vlad
Copy link
Copy Markdown
Member Author

Thanks!

harveyrendell pushed a commit to pushpay/redash that referenced this pull request Jan 8, 2025
* Snapshot: 23.11.0-dev

* Snapshot: 23.12.0-dev

* Snapshot: 24.01.0-dev

* Snapshot: 24.02.0-dev

* clickhouse: check for `exception` field in response

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Vladislav Denisov <denisov@sports.ru>
Co-authored-by: Guido Petri <18634426+guidopetri@users.noreply.github.com>
Co-authored-by: Peter Lee <yankeeguyu@gmail.com>
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 this pull request may close these issues.

5 participants