Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply query settings for empty table model #362

Merged

Conversation

alexsubota
Copy link
Contributor

Summary

Fix for query_settings
Currently, when a table is created, the query settings are not passed to the query, which causes an issue with the join_use_nulls setting (empty table have not nullable column, and then we cant insert null into this column). This PR fixes this behavior.

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

@alexsubota alexsubota changed the title Apply query settings for empty model Apply query settings for empty table model Sep 25, 2024
@genzgd
Copy link
Contributor

genzgd commented Sep 25, 2024

Is it possible to add a test case? It's probably okay but can't tell by looking whether (for example), the contract logic interferes with the query.

@alexsubota
Copy link
Contributor Author

i can create test model like this

{{
    config(
        materialized='table',
        query_settings={
            'join_use_nulls': 1
        }
    )
}}
select t2.id as test_id
from (select 1 as id) t1
         left join (select 2 as id) t2
on t1.id=t2.id

and check that test_id has a nullable type.
Is it ok?

@alexsubota
Copy link
Contributor Author

@genzgd check test please

@BentsiLeviav
Copy link
Contributor

@genzgd I checked locally and it doesn't collide with the contract (from a syntax point of view).

@BentsiLeviav BentsiLeviav merged commit 0198fc8 into ClickHouse:main Oct 30, 2024
21 checks passed
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.

4 participants