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

Incremental strategy insert_delete is not valid. use_lw_deletes Being ignored #133

Closed
apostoltego opened this issue Feb 1, 2023 · 16 comments
Assignees

Comments

@apostoltego
Copy link

apostoltego commented Feb 1, 2023

Hi There,

It seems that since version 1.3.0 the incremental strategy reference has changed but this isn't documented.
Any model using incremental_strategy='insert+delete' throws:
The incremental strategy 'insert_delete' is not valid for Clickhouse. This is on version 1.3.2/1.3.3

Changing the strategy to incremental_strategy='delete_insert' resolves this error however it wrongfully throws this message after:
dbt_clickhouse adapter: Lightweight deletes are not available, using legacy ClickHouse strategy

Lightweight deletes are enabled in our instance.
Screenshot 2023-02-01 at 19 49 11

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

Yes, I think this is a bug. Currently you must also set use_lw_deletes: true in the profile to avoid that error. It shouldn't throw that error if you have explicitly set the strategy.

@genzgd genzgd added the bug Something isn't working label Feb 1, 2023
@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

Btw there is a PR for the updated documentation. https://github.com/dbt-labs/docs.getdbt.com/pull/2626/files

@genzgd genzgd self-assigned this Feb 1, 2023
@apostoltego
Copy link
Author

We've actually set that parameter in the profile which is why this was weird.
Looking at this though I don't think it's actually being used but I'm not very familiar with the structure of the project.

Screenshot 2023-02-01 at 19 46 43

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

Are you using HTTP or the native interface?

@apostoltego
Copy link
Author

Native

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

So I can't reproduce this . . . the correct name is definitely delete+insert (or delete_insert). The profile setting does not look necessary if you set the strategy on the model and lightweight deletes are available, so what I thought was a bug actually works as intended. The lightweight delete test passes for both native and http. The code that throws that warning should only be failing if the connection has_lw_deletes is False:

        if not conn.handle.has_lw_deletes and strategy == 'delete_insert':
            logger.warning(
                'Lightweight deletes are not available, using legacy ClickHouse strategy'
            )
            strategy = 'legacy'

Can you tell if it is getting set correctly at the line of code you pasted above?

@apostoltego
Copy link
Author

Here's my profile definition:

  outputs:
    target: default
    default:
      type: clickhouse
      schema: default
      user: "{{ env_var('DBT_CLICKHOUSE_USER') }}"
      password: "{{ env_var('DBT_CLICKHOUSE_PASSWORD') }}"
      driver: native
      port: 9000
      host: "{{ env_var('DBT_CLICKHOUSE_HOST', 'localhost') }}"
      retries: 1
      verify: False
      secure: False
      connect_timeout: 15
      send_receive_timeout: 300
      sync_request_timeout: 5
      check_exchange: False
      threads: 12
      use_lw_deletes: True```
      
I'm not sure what's the best way to test whether the config is being picked properly. Do you have any recommendations? 

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

So you get the warning on 1.3.3?

If possible you could just hack a print statement into the source code, or you could check the ClickHouse query log for a query like SELECT value FROM system.settings WHERE name = 'allow_experimental_lightweight_delete'.

I just don't know what's happening since I can't reproduce locally.

@apostoltego
Copy link
Author

apostoltego commented Feb 1, 2023

The result of the query is 1
I've added prints as follows:

            lw_deletes = self.get_ch_setting('allow_experimental_lightweight_delete')
            print(f"lw_deletes after checking db: {lw_deletes}")
            self.has_lw_deletes = lw_deletes is not None and int(lw_deletes) > 0
            print(f"lw_deletes after checking self.has_lw_deletes: {self.has_lw_deletes}")

output is:

lw_deletes after checking db: 0
lw_deletes after checking self.has_lw_deletes: False

So looks like maybe the setting isn't cascading properly ?
Could this be tied to every user it's weird. To confirm, I'm setting this with the following:
set allow_experimental_lightweight_delete=1;

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

The setting can be per profile, per user, per session, or per query. If you just set that in a session with clickhouse-client it will only be for that session. You probably need to update the setting in either the user profile or user xml (or via the SQL cli) so that it "sticks".

@apostoltego
Copy link
Author

Gotcha - I'll give that a try tomorrow as there's active processes I can't stop right now. Closing as opening another sessions shows the value different. It's likely that!
Thanks a lot for your (very swift) help!

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

No problem . . . thanks for opening an issue. Other people may well run into the same thing and we should at a minimum document the process better.

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

The other option is to try to set it directly from dbt for the dbt session, so I'll investigate that for the next release.

@genzgd genzgd removed the bug Something isn't working label Feb 1, 2023
@apostoltego
Copy link
Author

@genzgd do you know if theres a specific way of applying this config to the users.xml?
Putting <allow_experimental_lightweight_delete>1</allow_experimental_lightweight_delete> there doesn't seem to do the trick. Other config items are loaded fine so this is a bit strange.

Appreciate this is going off topic now but hoping it's something you're familiar with 🙏

@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2023

That xml should go either either into a specific user element or profile element. For my local test server I put it in the "default" profile element in my users.xml:

 <profiles>
        <default>
           
            <!--<http_max_field_value_size>50</http_max_field_value_size>-->
            <allow_experimental_object_type>1</allow_experimental_object_type>
            <enable_http_compression>1</enable_http_compression>
            <cast_ipv4_ipv6_default_on_conversion_error>1</cast_ipv4_ipv6_default_on_conversion_error>
            <flatten_nested>0</flatten_nested>
            <input_format_skip_unknown_fields>0</input_format_skip_unknown_fields>
            <allow_experimental_lightweight_delete>1</allow_experimental_lightweight_delete>
            <query_profiler_real_time_period_ns>1000000000</query_profiler_real_time_period_ns>
            <query_profiler_cpu_time_period_ns>1000000000</query_profiler_cpu_time_period_ns>
        </default>
        <geoff>
            <http_max_field_value_size>350</http_max_field_value_size>
        </geoff>
    </profiles>

@apostoltego
Copy link
Author

Setting it on the user's block seems to have been my error. Setting it up on the profile worked - Thank you!

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

No branches or pull requests

2 participants