Clickhouse string ordering and string filtering by UTF8 instead of bytes #6143
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| import R from 'ramda'; | ||
|
|
||
| import { getEnv, parseSqlInterval } from '@cubejs-backend/shared'; | ||
| import { BaseQuery } from './BaseQuery'; | ||
| import { BaseFilter } from './BaseFilter'; | ||
| import { UserError } from '../compiler/UserError'; | ||
|
|
@@ -18,7 +20,7 @@ class ClickHouseFilter extends BaseFilter { | |
| public likeIgnoreCase(column, not, param, type) { | ||
| const p = (!type || type === 'contains' || type === 'ends') ? '%' : ''; | ||
| const s = (!type || type === 'contains' || type === 'starts') ? '%' : ''; | ||
| return `lowerUTF8(toValidUTF8(${column})) ${not ? 'NOT' : ''} LIKE CONCAT('${p}', lowerUTF8(toValidUTF8(${this.allocateParam(param)})), '${s}')`; | ||
| } | ||
|
|
||
| public castParameter() { | ||
|
|
@@ -123,7 +125,7 @@ export class ClickHouseQuery extends BaseQuery { | |
| .join(' AND '); | ||
| } | ||
|
|
||
| public getField(id) { | ||
| const equalIgnoreCase = (a, b) => ( | ||
| typeof a === 'string' && typeof b === 'string' && a.toUpperCase() === b.toUpperCase() | ||
| ); | ||
|
|
@@ -134,16 +136,30 @@ export class ClickHouseQuery extends BaseQuery { | |
| d => equalIgnoreCase(d.dimension, id), | ||
| ); | ||
|
|
||
| if (!field) { | ||
| field = this.measures.find( | ||
| d => equalIgnoreCase(d.measure, id) || equalIgnoreCase(d.expressionName, id), | ||
| ); | ||
| } | ||
|
|
||
| return field; | ||
| } | ||
|
|
||
| public getFieldAlias(id) { | ||
| const field = this.getField(id); | ||
|
|
||
| if (field) { | ||
| return field.aliasName(); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| public getFieldType(id) { | ||
| const field = this.getField(id); | ||
|
|
||
| if (field) { | ||
| return field.definition().type; | ||
| } | ||
|
|
||
| return null; | ||
|
|
@@ -168,6 +184,35 @@ export class ClickHouseQuery extends BaseQuery { | |
| return `${fieldAlias} ${direction}`; | ||
| } | ||
|
|
||
| public override orderBy() { | ||
|
There was a problem hiding this comment. Just to mention, it's not exactly clear to me how to fix it, so maybe comment with TODO/caveat would be enough It's not enough to override I think, Consider query like this coming to Cube's SQL API Inner query can (almost) be represented as regular query to Cube. cube/rust/cubesql/cubesql/src/compile/engine/df/wrapper.rs Lines 1415 to 1440 in 92a4b8e This code path will not use orderBy methods, but will use templates, that can be overridden by adapter.
But there's more! There was a problem hiding this comment. @mcheshkov As far as I can see To be honest, I got a little bit overwhelmed by cubesql code. It took sometime reading the code to understand what you meant. I think I've got to understand in which cases SQL API pushes down queries, and how SQL API handles sorting. |
||
| // | ||
| // ClickHouse orders string by bytes, so we need to use COLLATE 'en' to order by string | ||
| // | ||
| if (R.isEmpty(this.order)) { | ||
| return ''; | ||
| } | ||
|
|
||
| const collation = getEnv('clickhouseSortCollation'); | ||
|
|
||
| const orderByString = R.pipe( | ||
| R.map((order) => { | ||
| let orderString = this.orderHashToString(order); | ||
| if (this.getFieldType(order) === 'string') && collation !== '' { | ||
|
There was a problem hiding this comment. Sorry for the mess, after thinking for a while, I agree with your original suggestion to fall back to There was a problem hiding this comment. As written before it didn't allow removing I, personally, would not expect it to use collation by default, but I also find default collations in PostgreSQL or MySQL strange, so maybe it's just me. There was a problem hiding this comment. Something I wrote when reviewing previous version: Any collation in order by makes ordering case-insensitive in ClickHouse, and it would not match Consider query like this: With Also keep in mind that Cube can issue ungrouped queries with order and limit, that would need to sort very large set of rows, so performance impact from enabling it can (I think) be huge in some specific cases, and specifically it can be unexpected for ClickHouse There was a problem hiding this comment. I agree on being able to disable collation behavior. We might document it to set the env variable to a specific value. Or I might add an another env variable. The problem with Clickhouse is that it only allows collation in the order by clause. Nothing else, so there is no database level, table level, column level, session level collation setting anywhere. Otherwise I would agree. |
||
| orderString = `${orderString} COLLATE '${collation}'`; | ||
| } | ||
| return orderString; | ||
| }), | ||
| R.reject(R.isNil), | ||
| R.join(', ') | ||
| )(this.order); | ||
|
|
||
| if (!orderByString) { | ||
| return ''; | ||
| } | ||
|
|
||
| return ` ORDER BY ${orderByString}`; | ||
| } | ||
|
|
||
| public groupByClause() { | ||
| if (this.ungrouped) { | ||
| return ''; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use
ILIKEinstead oftoValidUTF8? I mean, it was written like this, but since you are messing around here - why not? BothlowerUTF8andILIKEcan handle UTF-8, butILIKEis a bit more direct, IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if ILIKE is UTF-8 compatible. It might just be calling lower(). Let me ask this to the Clickhouse folk first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, for UTF-8 strings
_has a "single code point" meaninghttps://clickhouse.com/docs/en/sql-reference/functions/string-search-functions#ilike