Skip to content

Conversation

@herbert-allium
Copy link
Contributor

@herbert-allium herbert-allium commented Dec 17, 2025


Note

Adds dictionary materialization options and tests to better align with ClickHouse dictionary refresh mechanics.

  • In dictionary.sql, makes LIFETIME(...) optional and adds update_field and update_lag support within CLICKHOUSE(...) source; preserves RANGE(...) handling
  • New integration tests for DIRECT layout dictionaries and for update_field/update_lag behavior; adds people_updated seed and schema entry

Written by Cursor Bugbot for commit 8912b64. This will update automatically on new commits. Configure here.

Copy link
Contributor

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

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

Looks good to me 🤙

💡 To request another review, post a new comment with "/windsurf-review".

@koletzilla
Copy link
Contributor

Hi @herbert-allium, thanks for the contribution! Would you also add new tests to cover these cases? You may be able to add them following the existing ones in this file https://github.com/ClickHouse/dbt-clickhouse/blob/main/tests/integration/adapter/dictionary/test_dictionary.py

@koletzilla
Copy link
Contributor

Comment to leave it as reference: this PR closes #352

@herbert-allium
Copy link
Contributor Author

hey @koletzilla sorry for the late reply! I added some tests, can you check if they are good?

I didn't fully test the update_field and update_age behaviours because that'll require mocking the current time.

{%- endif %}
{%- if config.get('update_lag') %}
update_lag {{ config.get('update_lag') }}
{%- endif %}
Copy link

Choose a reason for hiding this comment

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

Falsy update_lag value silently omitted from generated SQL

Medium Severity

The condition config.get('update_lag') will evaluate to False when update_lag=0 is configured, because 0 is falsy in Jinja2. This causes the update_lag clause to be silently omitted from the generated SQL, even though 0 is a valid value meaning "no delay before reload." The existing code on line 82 uses is not none for similar checks, which is the correct pattern to handle numeric values including zero.

Fix in Cursor Fix in Web

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