Skip to content

[PLAT-435] allow dict rewrites in ch without create temp table privileges, add docs#9020

Merged
pjain1 merged 2 commits intomainfrom
ch_dict_improvements
Mar 13, 2026
Merged

[PLAT-435] allow dict rewrites in ch without create temp table privileges, add docs#9020
pjain1 merged 2 commits intomainfrom
ch_dict_improvements

Conversation

@pjain1
Copy link
Member

@pjain1 pjain1 commented Mar 11, 2026

  • Add EscapeQualifiedIdentifier to escape dot-separated identifiers (e.g. db.table) which may be provided for lookup_table property.
  • Change LookupSelectExpr to query the dictionary table directly instead of using dictionary() table function, which requires CREATE TEMPORARY TABLE privilege.
  • Docs for lookup feature.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

switch d {
case DialectClickHouse:
return fmt.Sprintf("SELECT %s FROM dictionary(%s)", d.EscapeIdentifier(lookupKeyColumn), d.EscapeIdentifier(lookupTable)), nil
return fmt.Sprintf("SELECT %s FROM %s", d.EscapeIdentifier(lookupKeyColumn), d.EscapeQualifiedIdentifier(lookupTable)), nil
Copy link
Member Author

@pjain1 pjain1 Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now this directly reads from the dictionary table, may be we should make this configurable if the dictionary table as dictionary() may directly read from memory if the dict is loaded in memory so a minor optimization probably.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this issue, why can't we keep dictionary(%s)? And how would you propose to make it configurable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using dictionary() is the main issue for which PR is raised, its a table function and on the table functions page it says You can't use table functions if the [allow_ddl](https://clickhouse.com/docs/operations/settings/settings#allow_ddl) setting is disabled. Customer queries are failing with

Not enough privileges. To execute this query, it's necessary to have the grant CREATE TEMPORARY TABLE ON *.*. (ACCESS_DENIED)

Thus directly reading from the underlying dictionary table. By making it configurable I meant using an instance config flag using which we will choose if to use table function or directly read from dict table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to avoid a config flag for such an edge case if possible. But it's okay if it's needed.

Does dictionary() impact performance a lot? If it does, then maybe we should just tell users that Clickhouse requires DDL access to use dictionaries in Rill? If it doesn't, then I think this is fine and we can just not use the table func.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have empirical evidence around performance as in practice dictionary tables are really small so could not see much difference. I think its fine to keep this not configurable.

switch d {
case DialectClickHouse:
return fmt.Sprintf("SELECT %s FROM dictionary(%s)", d.EscapeIdentifier(lookupKeyColumn), d.EscapeIdentifier(lookupTable)), nil
return fmt.Sprintf("SELECT %s FROM %s", d.EscapeIdentifier(lookupKeyColumn), d.EscapeQualifiedIdentifier(lookupTable)), nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of automatically parsing dots in lookup_table, should we add an optional lookup_database property since clickhouse table names can have dots in them. Since its very rare and may also require quoting full table name while doing dictGet, I just kept it simple.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to keep it simple – see my other comment

@pjain1 pjain1 requested a review from begelundmuller March 12, 2026 04:26
switch d {
case DialectClickHouse:
return fmt.Sprintf("SELECT %s FROM dictionary(%s)", d.EscapeIdentifier(lookupKeyColumn), d.EscapeIdentifier(lookupTable)), nil
return fmt.Sprintf("SELECT %s FROM %s", d.EscapeIdentifier(lookupKeyColumn), d.EscapeQualifiedIdentifier(lookupTable)), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to keep it simple – see my other comment

switch d {
case DialectClickHouse:
return fmt.Sprintf("SELECT %s FROM dictionary(%s)", d.EscapeIdentifier(lookupKeyColumn), d.EscapeIdentifier(lookupTable)), nil
return fmt.Sprintf("SELECT %s FROM %s", d.EscapeIdentifier(lookupKeyColumn), d.EscapeQualifiedIdentifier(lookupTable)), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this issue, why can't we keep dictionary(%s)? And how would you propose to make it configurable?

@pjain1 pjain1 requested a review from begelundmuller March 13, 2026 04:37
@pjain1 pjain1 merged commit 23aee08 into main Mar 13, 2026
19 of 21 checks passed
@pjain1 pjain1 deleted the ch_dict_improvements branch March 13, 2026 13:17
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.

2 participants