dbt-clickhouse icon indicating copy to clipboard operation
dbt-clickhouse copied to clipboard

Filter settings based on Engine

Open shachibista opened this issue 1 year ago • 2 comments

This PR introduces a feature that ignores settings that are not compatible with the Engine. More details in this issue: https://github.com/ClickHouse/dbt-clickhouse/issues/366

Checklist

  • [x] A human-readable description of the changes was provided to include in CHANGELOG

shachibista avatar Oct 11 '24 16:10 shachibista

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 11 '24 16:10 CLAassistant

Hi @shachibista,

Thank you for your contribution!

I'm hesitant to move forward with this change as it introduces ongoing maintenance requirements. With this setup, every time ClickHouse adds new settings, we’ll need to update the ENGINE_SETTINGS dictionary. Similarly, any new engine additions would also require corresponding updates.

I think the liability is greater than the benefit.

BentsiLeviav avatar Oct 30 '24 13:10 BentsiLeviav

@genzgd suggested changing it from a white list to a black list (remove settings we identified as not relevant for this engine), and in case we face a new engine, pass the settings as is.

@shachibista would you mind changing the logic to support this?

BentsiLeviav avatar Nov 03 '24 12:11 BentsiLeviav

Let me know WDYT about changing it to a black list instead of a white list

I don't personally agree with a black-list since:

  1. It also requires some maintainance so the above reasoning about maintainence is moot.
  2. It requires invalid settings to be discovered by users for each engine, forcing them to wait for the next release of the library.

However I have made the requested changes if it feels right to you since the original issue I was trying to solve was solved using another method (wrote own observability package, for posterity) and as such this issue is no longer important to me at this moment.

Hope that helps!

shachibista avatar Jan 06 '25 23:01 shachibista

It also requires some maintainance so the above reasoning about maintainence is moot.

There is a significant difference between updating the extensive settings dictionary with every ClickHouse (CH) release and allowing users to be completely blocked from using new configurations until we update it. By addressing mismatched settings, we can handle far fewer scenarios compared to CH introducing tens of new configurations every year.

forcing them to wait for the next release of the library.

In my opinion, this approach is better than blocking the entire community from accessing new features or settings, which happens with every release.

However I have made the requested changes if it feels right to you since the original issue I was trying to solve was solved using another method (wrote own observability package, for posterity) and as such this issue is no longer important to me at this moment.

Thank you for changing the code. Your efforts are sincerely appreciated and not taken for granted.

BentsiLeviav avatar Jan 07 '25 08:01 BentsiLeviav