fix(time grain): time grain clickhouse db error #26735
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #26735 +/- ##
==========================================
+ Coverage 69.07% 69.13% +0.06%
==========================================
Files 1930 1930
Lines 75279 75010 -269
Branches 8429 8429
==========================================
- Hits 51999 51859 -140
+ Misses 21133 21004 -129
Partials 2147 2147
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@hxxdraised please post the original SQL and fixed SQL in the PR description for the reviewing. I guess that this issue is an known issue in CH SQL parser side, but there is a workaround to fix it in Superset side. To create a verbose name in the Dataset level in Superset, and then the SQL snippet will be 
|
There was a problem hiding this comment.
Thanks @hxxdraised for the PR. Your change augments some existing shared logic in the superset/models/helpers.py which I'm worried could break existing logic.
Would you mind adding some unit tests? This would help reviewers provide the necessary level of confidence that said change is valid.
| @@ -397,6 +397,9 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods | |||
| # Can the catalog be changed on a per-query basis? | |||
| supports_dynamic_catalog = False | |||
|
|
|||
| # Use column alias instead of aggregate function in GROUP BY | |||
There was a problem hiding this comment.
| # Use column alias instead of aggregate function in GROUP BY | |
| # Use column alias instead of aggregate function in GROUP BY |
| @@ -397,6 +397,9 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods | |||
| # Can the catalog be changed on a per-query basis? | |||
| supports_dynamic_catalog = False | |||
|
|
|||
| # Use column alias instead of aggregate function in GROUP BY | |||
| use_column_alias_in_groupby = False | |||
There was a problem hiding this comment.
I'm surprised that the SQLAlchemy dialect doesn't handle this. The visit_label method is likely relevant. Ideally we adopt the "shift left" mentality and have the underlying dialect (rather than Superset) handle this logic.
|
@john-bodley @betodealmeida Should not we somehow connect this PR with SIPs 115 and 117? |
|
@TechAuditBI this change seems unrelated to SIP-115 and SIP-117. Personally—if we adopt the "shift left" mentatility—th necessary change should be made in the clickhouse-connect SQLAlchemy dialect. |
|
Hi all! Just wondering if there's anyone sees a way to move forward on this, and close out the issue (which is getting pretty stale). I'm also wondering if this is even still an issue, since |
|
Hi @rusackas , I use clickhouse (v 5.7.30) with superset master and this isn't a problem for me with clickhouse-connect 0.7.16
|
|
Thanks @round3d ! I'll go ahead and close this PR and the related issue. If anyone has issues and sees the need to reopen this, say the word! |
SUMMARY
DB error on chart load when time granularity is set when the source is Clickhouse. Error example:
Error: Orig exception: Code: 215. DB::Exception: Column `Column2` is not under aggregate function and not in GROUP BY: While processing toStartOfDay(toDateTime(Column2)) AS Column2, count() AS count.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before:
after:
TESTING INSTRUCTIONS
clickhouse-sqlalchemy==0.2.5line indocker/requirements-local.txtADDITIONAL INFORMATION