Clickhouse string ordering and string filtering by UTF8 instead of bytes #6143
Conversation
github-actions
bot
added
the
pr:community
|
Hey @casab ! Thanks for contributing! Could you please add an integration test for it? |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 8 Skipped Deployments
|
casab
force-pushed
the
feature/clickhouse_utf8_filter_order
branch
from
645512c to
63b935c
Compare
|
@casab is attempting to deploy a commit to the Cube Dev Team on Vercel. A member of the Team first needs to authorize it. |
…e_utf8_filter_order
|
@casab May I kindly ask you to test with the latest release and rebase your changes on top of it? We have migrated to a new ClickHouse client library recently. Thanks in advance! |
|
@igorlukanin Can you approve running the workflows? |
|
Hi @casab Could you please sync with the latest master, resolve conflicts and fix warnings/errors? Ping me whenever you need to approve running workflows! |
|
@KSDaemon of course, done it. |
|
@casab Hey! Thnx for the updates! Unfortunately, some lint errors still exists: Maybe thats because of merge... But anyway... Can you fix them, please? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6143 +/- ##
==========================================
- Coverage 83.57% 80.73% -2.84%
==========================================
Files 227 227
Lines 81618 81646 +28
==========================================
- Hits 68210 65916 -2294
- Misses 13408 15730 +2322
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@KSDaemon @mcheshkov Hey, I have fixed all the errors. And implemented all the required parts. I would appreciate it if you can review it. Especially the rust part. |
| @@ -123,7 +125,7 @@ export class ClickHouseQuery extends BaseQuery { | |||
| .join(' AND '); | |||
| } | |||
|
|
|||
| public getFieldAlias(id) { | |||
| public getField(id) { | |||
There was a problem hiding this comment.
Do we need to make it public? Maybe private would be better?
| @@ -168,6 +188,43 @@ export class ClickHouseQuery extends BaseQuery { | |||
| return `${fieldAlias} ${direction}`; | |||
| } | |||
|
|
|||
| public getCollation() { | |||
There was a problem hiding this comment.
And the same question here: should it be public?
| if (R.isEmpty(this.order)) { | ||
| return ''; | ||
| } | ||
|
|
||
| const collation = this.getCollation(); | ||
|
|
||
| const orderByString = R.pipe( | ||
| R.map((order) => { | ||
| let orderString = this.orderHashToString(order); | ||
| if (collation && this.getFieldType(order) === 'string') { | ||
| orderString = `${orderString} COLLATE '${collation}'`; | ||
| } | ||
| return orderString; | ||
| }), | ||
| R.reject(R.isNil), | ||
| R.join(', ') | ||
| )(this.order); |
There was a problem hiding this comment.
Don't you mind rewriting this in plain JS instead of using Ramda?
Smth like this:
| if (R.isEmpty(this.order)) { | |
| return ''; | |
| } | |
| const collation = this.getCollation(); | |
| const orderByString = R.pipe( | |
| R.map((order) => { | |
| let orderString = this.orderHashToString(order); | |
| if (collation && this.getFieldType(order) === 'string') { | |
| orderString = `${orderString} COLLATE '${collation}'`; | |
| } | |
| return orderString; | |
| }), | |
| R.reject(R.isNil), | |
| R.join(', ') | |
| )(this.order); | |
| if (this.order.length === 0) { | |
| return ''; | |
| } | |
| const collation = this.getCollation(); | |
| const orderByString = this.order | |
| .map((order) => { | |
| let orderString = this.orderHashToString(order); | |
| if (collation && this.getFieldType(order) === 'string') { | |
| orderString = `${orderString} COLLATE '${collation}'`; | |
| } | |
| return orderString; | |
| }) | |
| .filter(Boolean) // Analogue `R.reject(R.isNil)` | |
| .join(', '); |
|
@casab We're actively looking into merging this PR. Could you please resolve the conflicts and resolve on top of master once again? Thanks in advance! |
|
@igorlukanin sure, I'll look into it and let you know. |
| //let data_type = expr.get_type(&schema.clone())?; | ||
| //let data_type = Self::generate_sql_type(generator.clone(), data_type.clone())?; |
There was a problem hiding this comment.
Could you please remove these comments? They are likely experimentation leftovers. Thanks in advance! 😊
Other than that, great PR! I feel like this can be merged once other suggested changes are resolved or agreed upon. Thanks!
| const oldOrderBy = '{% if order_by %}\nORDER BY {{ order_by | map(attribute=\'expr\') | join(\', \') }}{% endif %}'; | ||
|
|
||
| const newOrderBy = | ||
| '{% if order_by %}\nORDER BY {% for item in order_by %}{{ item.expr }}' + | ||
| `{%- if item.data_type and item.data_type == 'string' %} COLLATE '${collation}'{% endif %}` + | ||
| '{%- if not loop.last %}, {% endif %}{% endfor %}{% endif %}'; |
There was a problem hiding this comment.
I suggest staying away from replacing strings by putting the original ORDER BY expression here. The statements/select template might change over time in BaseQuery, and if ORDER BY condition is touched, this code would break.
I would suggest moving whole statements/select template here, then deciding the ORDER BY part based on the provided condition. This way if BaseQuery' statements/select is modified, this dialect would be untouched and would continue working regardless. We already use similar approach with overriding whole statements/select template in PinotQuery and PrestodbQuery.
Clickhouse defaults to using bytes to order by and string manipulation functions such as
lower,upperuses ascii. To overcome this limitation they haveCOLLATEkeyword, andlowerUTF8,upperUTF8functions.Check List
Description of Changes Made (if issue reference is not provided)
CONCATSQL function with js template literal to prevent unnecessary DB function calllowerUTF8instead oflowerto support utf8 compatible searchCOLLATE ‘en’when ordering by strings to order incasesensitive. Clickhouse orders by bytes on default.