Filter settings based on Engine
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
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.
@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?
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:
- It also requires some maintainance so the above reasoning about maintainence is moot.
- 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!
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.